Skip to content

Commit bea183d

Browse files
authored
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 RaspberryPiFoundation/blockly-keyboard-experimentation#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 RaspberryPiFoundation/blockly-keyboard-experimentation#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). RaspberryPiFoundation/blockly-keyboard-experimentation#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.
1 parent c0489b4 commit bea183d

File tree

7 files changed

+102
-33
lines changed

7 files changed

+102
-33
lines changed

core/dropdowndiv.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -346,8 +346,8 @@ function showPositionedByRect(
346346
secondaryX,
347347
secondaryY,
348348
manageEphemeralFocus,
349-
autoCloseOnLostFocus,
350349
opt_onHide,
350+
autoCloseOnLostFocus,
351351
);
352352
}
353353

@@ -366,9 +366,9 @@ function showPositionedByRect(
366366
* @param primaryY Desired origin point y, in absolute px.
367367
* @param secondaryX Secondary/alternative origin point x, in absolute px.
368368
* @param secondaryY Secondary/alternative origin point y, in absolute px.
369-
* @param opt_onHide Optional callback for when the drop-down is hidden.
370369
* @param manageEphemeralFocus Whether ephemeral focus should be managed
371370
* according to the widget div's lifetime.
371+
* @param opt_onHide Optional callback for when the drop-down is hidden.
372372
* @param autoCloseOnLostFocus Whether the drop-down should automatically hide
373373
* if it loses DOM focus for any reason.
374374
* @returns True if the menu rendered at the primary origin point.
@@ -382,8 +382,8 @@ export function show<T>(
382382
secondaryX: number,
383383
secondaryY: number,
384384
manageEphemeralFocus: boolean,
385-
autoCloseOnLostFocus: boolean,
386385
opt_onHide?: () => void,
386+
autoCloseOnLostFocus?: boolean,
387387
): boolean {
388388
owner = newOwner as Field;
389389
onHide = opt_onHide || null;

core/focus_manager.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -138,10 +138,10 @@ export class FocusManager {
138138
element instanceof Node &&
139139
ephemeralFocusElem.contains(element);
140140
if (hadFocus !== hasFocus) {
141+
this.ephemerallyFocusedElementCurrentlyHasFocus = hasFocus;
141142
if (this.ephemeralDomFocusChangedCallback) {
142143
this.ephemeralDomFocusChangedCallback(hasFocus);
143144
}
144-
this.ephemerallyFocusedElementCurrentlyHasFocus = hasFocus;
145145
}
146146
}
147147
};

core/widgetdiv.ts

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -99,13 +99,16 @@ export function createDom() {
9999
* passed in here then callers should manage ephemeral focus directly
100100
* otherwise focus may not properly restore when the widget closes. Defaults
101101
* to true.
102+
* @param autoCloseOnLostFocus Whether the widget should automatically hide if
103+
* it loses DOM focus for any reason.
102104
*/
103105
export function show(
104106
newOwner: unknown,
105107
rtl: boolean,
106108
newDispose: () => void,
107109
workspace?: WorkspaceSvg | null,
108110
manageEphemeralFocus: boolean = true,
111+
autoCloseOnLostFocus: boolean = true,
109112
) {
110113
hide();
111114
owner = newOwner;
@@ -131,7 +134,18 @@ export function show(
131134
dom.addClass(div, themeClassName);
132135
}
133136
if (manageEphemeralFocus) {
134-
returnEphemeralFocus = getFocusManager().takeEphemeralFocus(div);
137+
const autoCloseCallback = autoCloseOnLostFocus
138+
? (hasFocus: boolean) => {
139+
// If focus is ever lost, close the widget.
140+
if (!hasFocus) {
141+
hide();
142+
}
143+
}
144+
: null;
145+
returnEphemeralFocus = getFocusManager().takeEphemeralFocus(
146+
div,
147+
autoCloseCallback,
148+
);
135149
}
136150
}
137151

tests/mocha/dropdowndiv_test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -155,7 +155,7 @@ suite('DropDownDiv', function () {
155155
});
156156
test('Escape dismisses DropDownDiv', function () {
157157
let hidden = false;
158-
Blockly.DropDownDiv.show(this, false, 0, 0, 0, 0, false, false, () => {
158+
Blockly.DropDownDiv.show(this, false, 0, 0, 0, 0, false, () => {
159159
hidden = true;
160160
});
161161
assert.isFalse(hidden);
@@ -276,7 +276,7 @@ suite('DropDownDiv', function () {
276276
// Focus an element outside of the drop-down.
277277
document.getElementById('nonTreeElementForEphemeralFocus').focus();
278278

279-
// the drop-down should now be hidden since it lost focus.
279+
// The drop-down should now be hidden since it lost focus.
280280
const dropDownDivElem = document.querySelector('.blocklyDropDownDiv');
281281
assert.strictEqual(dropDownDivElem.style.opacity, '0');
282282
});

tests/mocha/focus_manager_test.js

Lines changed: 14 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -5624,21 +5624,6 @@ suite('FocusManager', function () {
56245624
/* Ephemeral focus tests. */
56255625

56265626
suite('takeEphemeralFocus()', function () {
5627-
setup(function () {
5628-
// Ensure ephemeral-specific elements are focusable.
5629-
document.getElementById('nonTreeElementForEphemeralFocus').tabIndex = -1;
5630-
document.getElementById('nonTreeGroupForEphemeralFocus').tabIndex = -1;
5631-
});
5632-
teardown(function () {
5633-
// Ensure ephemeral-specific elements have their tab indexes reset for a clean state.
5634-
document
5635-
.getElementById('nonTreeElementForEphemeralFocus')
5636-
.removeAttribute('tabindex');
5637-
document
5638-
.getElementById('nonTreeGroupForEphemeralFocus')
5639-
.removeAttribute('tabindex');
5640-
});
5641-
56425627
test('with no focused node does not change states', function () {
56435628
this.focusManager.registerTree(this.testFocusableTree2);
56445629
this.focusManager.registerTree(this.testFocusableGroup2);
@@ -6070,7 +6055,7 @@ suite('FocusManager', function () {
60706055
assert.isTrue(callback.thirdCall.calledWithExactly(true));
60716056
});
60726057

6073-
test('with focus change callback set focus to non-ephemeral element with auto return finishes ephemeral', function () {
6058+
test('with focus change callback set focus to non-ephemeral element with auto return finishes ephemeral does not restore to focused node', function () {
60746059
this.focusManager.registerTree(this.testFocusableTree2);
60756060
this.focusManager.registerTree(this.testFocusableGroup2);
60766061
this.focusManager.focusNode(this.testFocusableTree2Node1);
@@ -6090,21 +6075,24 @@ suite('FocusManager', function () {
60906075
// Force focus away, triggering the callback's automatic returning logic.
60916076
ephemeralElement2.focus();
60926077

6093-
// The original focused node should be restored.
6094-
const nodeElem = this.testFocusableTree2Node1.getFocusableElement();
6078+
// The original node should not be focused since the ephemeral element
6079+
// lost its own DOM focus while ephemeral focus was active. Instead, the
6080+
// newly active element should still hold focus.
60956081
const activeElems = Array.from(
60966082
document.querySelectorAll(ACTIVE_FOCUS_NODE_CSS_SELECTOR),
60976083
);
6098-
assert.strictEqual(
6099-
this.focusManager.getFocusedNode(),
6100-
this.testFocusableTree2Node1,
6084+
const passiveElems = Array.from(
6085+
document.querySelectorAll(PASSIVE_FOCUS_NODE_CSS_SELECTOR),
61016086
);
6102-
assert.strictEqual(activeElems.length, 1);
6087+
assert.isEmpty(activeElems);
6088+
assert.strictEqual(passiveElems.length, 1);
61036089
assert.includesClass(
6104-
nodeElem.classList,
6105-
FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME,
6090+
this.testFocusableTree2Node1.getFocusableElement().classList,
6091+
FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME,
61066092
);
6107-
assert.strictEqual(document.activeElement, nodeElem);
6093+
assert.isNull(this.focusManager.getFocusedNode());
6094+
assert.strictEqual(document.activeElement, ephemeralElement2);
6095+
assert.isFalse(this.focusManager.ephemeralFocusTaken());
61086096
});
61096097

61106098
test('with focus on non-ephemeral element ephemeral ended does not restore to focused node', function () {
@@ -6139,6 +6127,7 @@ suite('FocusManager', function () {
61396127
this.testFocusableTree2Node1.getFocusableElement().classList,
61406128
FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME,
61416129
);
6130+
assert.isNull(this.focusManager.getFocusedNode());
61426131
assert.strictEqual(document.activeElement, ephemeralElement2);
61436132
assert.isFalse(this.focusManager.ephemeralFocusTaken());
61446133
});

tests/mocha/index.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,7 @@
142142
</text>
143143
</g>
144144
</g>
145-
<g id="nonTreeGroupForEphemeralFocus"></g>
145+
<g id="nonTreeGroupForEphemeralFocus" tabindex="-1"></g>
146146
</svg>
147147
<!-- Load mocha et al. before Blockly and the test modules so that
148148
we can safely import the test modules that make calls

tests/mocha/widget_div_test.js

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -444,5 +444,71 @@ suite('WidgetDiv', function () {
444444
assert.strictEqual(Blockly.getFocusManager().getFocusedNode(), block);
445445
assert.strictEqual(document.activeElement, blockFocusableElem);
446446
});
447+
448+
test('without auto close on lost focus lost focus does not hide widget div', function () {
449+
const block = this.setUpBlockWithField();
450+
const field = Array.from(block.getFields())[0];
451+
Blockly.getFocusManager().focusNode(block);
452+
Blockly.WidgetDiv.show(field, false, () => {}, null, true, false);
453+
454+
// Focus an element outside of the widget.
455+
document.getElementById('nonTreeElementForEphemeralFocus').focus();
456+
457+
// Even though the widget lost focus, it should still be visible.
458+
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
459+
assert.strictEqual(widgetDivElem.style.display, 'block');
460+
});
461+
462+
test('with auto close on lost focus lost focus hides widget div', function () {
463+
const block = this.setUpBlockWithField();
464+
const field = Array.from(block.getFields())[0];
465+
Blockly.getFocusManager().focusNode(block);
466+
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
467+
468+
// Focus an element outside of the widget.
469+
document.getElementById('nonTreeElementForEphemeralFocus').focus();
470+
471+
// The widget should now be hidden since it lost focus.
472+
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
473+
assert.strictEqual(widgetDivElem.style.display, 'none');
474+
});
475+
476+
test('with auto close on lost focus lost focus with nested div hides widget div', function () {
477+
const block = this.setUpBlockWithField();
478+
const field = Array.from(block.getFields())[0];
479+
Blockly.getFocusManager().focusNode(block);
480+
const nestedDiv = document.createElement('div');
481+
nestedDiv.tabIndex = -1;
482+
Blockly.WidgetDiv.getDiv().appendChild(nestedDiv);
483+
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
484+
nestedDiv.focus(); // It's valid to focus this during ephemeral focus.
485+
486+
// Focus an element outside of the widget.
487+
document.getElementById('nonTreeElementForEphemeralFocus').focus();
488+
489+
// The widget should now be hidden since it lost focus.
490+
const widgetDivElem = document.querySelector('.blocklyWidgetDiv');
491+
assert.strictEqual(widgetDivElem.style.display, 'none');
492+
});
493+
494+
test('with auto close on lost focus lost focus with nested div does not restore DOM focus', function () {
495+
const block = this.setUpBlockWithField();
496+
const field = Array.from(block.getFields())[0];
497+
Blockly.getFocusManager().focusNode(block);
498+
const nestedDiv = document.createElement('div');
499+
nestedDiv.tabIndex = -1;
500+
Blockly.WidgetDiv.getDiv().appendChild(nestedDiv);
501+
Blockly.WidgetDiv.show(field, false, () => {}, null, true, true);
502+
nestedDiv.focus(); // It's valid to focus this during ephemeral focus.
503+
504+
// Focus an element outside of the widget.
505+
const elem = document.getElementById('nonTreeElementForEphemeralFocus');
506+
elem.focus();
507+
508+
// Auto hiding should not restore focus back to the block since ephemeral
509+
// focus was lost before it was returned.
510+
assert.isNull(Blockly.getFocusManager().getFocusedNode());
511+
assert.strictEqual(document.activeElement, elem);
512+
});
447513
});
448514
});

0 commit comments

Comments
 (0)