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.
This commit is contained in:
Ben Henning
2025-07-08 16:06:24 -07:00
committed by GitHub
parent c0489b41e0
commit bea183d85d
7 changed files with 102 additions and 33 deletions

View File

@@ -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<T>(
secondaryX: number,
secondaryY: number,
manageEphemeralFocus: boolean,
autoCloseOnLostFocus: boolean,
opt_onHide?: () => void,
autoCloseOnLostFocus?: boolean,
): boolean {
owner = newOwner as Field;
onHide = opt_onHide || null;

View File

@@ -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;
}
}
};

View File

@@ -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,
);
}
}

View File

@@ -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');
});

View File

@@ -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());
});

View File

@@ -142,7 +142,7 @@
</text>
</g>
</g>
<g id="nonTreeGroupForEphemeralFocus"></g>
<g id="nonTreeGroupForEphemeralFocus" tabindex="-1"></g>
</svg>
<!-- Load mocha et al. before Blockly and the test modules so that
we can safely import the test modules that make calls

View File

@@ -444,5 +444,71 @@ suite('WidgetDiv', function () {
assert.strictEqual(Blockly.getFocusManager().getFocusedNode(), block);
assert.strictEqual(document.activeElement, blockFocusableElem);
});
test('without auto close on lost focus lost focus does not hide widget div', function () {
const block = this.setUpBlockWithField();
const field = Array.from(block.getFields())[0];
Blockly.getFocusManager().focusNode(block);
Blockly.WidgetDiv.show(field, false, () => {}, null, true, false);
// Focus an element outside of the widget.
document.getElementById('nonTreeElementForEphemeralFocus').focus();
// Even though the widget lost focus, it should still be visible.
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
assert.strictEqual(widgetDivElem.style.display, 'block');
});
test('with auto close on lost focus lost focus hides widget div', function () {
const block = this.setUpBlockWithField();
const field = Array.from(block.getFields())[0];
Blockly.getFocusManager().focusNode(block);
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
// Focus an element outside of the widget.
document.getElementById('nonTreeElementForEphemeralFocus').focus();
// The widget should now be hidden since it lost focus.
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
assert.strictEqual(widgetDivElem.style.display, 'none');
});
test('with auto close on lost focus lost focus with nested div hides widget div', function () {
const block = this.setUpBlockWithField();
const field = Array.from(block.getFields())[0];
Blockly.getFocusManager().focusNode(block);
const nestedDiv = document.createElement('div');
nestedDiv.tabIndex = -1;
Blockly.WidgetDiv.getDiv().appendChild(nestedDiv);
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
nestedDiv.focus(); // It's valid to focus this during ephemeral focus.
// Focus an element outside of the widget.
document.getElementById('nonTreeElementForEphemeralFocus').focus();
// The widget should now be hidden since it lost focus.
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
assert.strictEqual(widgetDivElem.style.display, 'none');
});
test('with auto close on lost focus lost focus with nested div does not restore DOM focus', function () {
const block = this.setUpBlockWithField();
const field = Array.from(block.getFields())[0];
Blockly.getFocusManager().focusNode(block);
const nestedDiv = document.createElement('div');
nestedDiv.tabIndex = -1;
Blockly.WidgetDiv.getDiv().appendChild(nestedDiv);
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
nestedDiv.focus(); // It's valid to focus this during ephemeral focus.
// Focus an element outside of the widget.
const elem = document.getElementById('nonTreeElementForEphemeralFocus');
elem.focus();
// Auto hiding should not restore focus back to the block since ephemeral
// focus was lost before it was returned.
assert.isNull(Blockly.getFocusManager().getFocusedNode());
assert.strictEqual(document.activeElement, elem);
});
});
});