Skip to content

Commit 0e16b04

Browse files
authored
fix: Auto close drop-down divs on lost focus (reapply) (#9213)
## 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 introduces support in `FocusManager` to receive feedback on when an ephemerally focused element entirely loses focus (that is, neither it nor any of its descendants have focus). This also introduces a behavior change for drop-down divs using the previously mentioned functionality to automatically close themselves when they lose focus for any reason (e.g. clicking outside of the div or tab navigating away from it). Finally, and **importantly**, this adds a case where ephemeral focus does _not_ automatically return to the previously focused node: when focus is lost to the ephemerally focused element's tree and isn't instead put on another focused node. ### Reason for Changes Ultimately, focus is probably the best proxy for cases when a drop-down div ought to no longer be open. However, tracking focus only within the scope of the drop-down div utility is rather difficult since a lot of the same problems that `FocusManager` handles also occur here (with regards to both descendants and outside elements receiving focus). It made more sense to expand `FocusManager`'s ephemeral focus support: - It was easier to implement this `FocusManager` and in a way that's much more robust (since it's leveraging existing event handlers). - Using `FocusManager` trivialized the solution for drop-down divs. - There could be other use cases where custom ephemeral focus uses might benefit from knowing when they lose focus. This new support is enabled by default for all drop-down divs, but can be disabled by callers if they wish to revert to the previous behavior of not auto-closing. The change for whether to restore ephemeral focus was needed to fix a drawback that arises from the automatic returning of ephemeral focus introduced in this PR: when a user clicks out of an open drop-down menu it will restore focus back to the node that held focus prior to taking ephemeral focus (since it properly hides the drop-down div and restores focus). This creates awkward behavior issues for both mouse and keyboard users: - For mouse: trying to open a drop-down outside of Blockly will automatically close the drop-down when the Blockly drop-down finishes closing (since focus is stolen back away from the thing the user clicked on). - For keyboard: tab navigating out of Blockly tries to force focus back to Blockly. **New in v2 of this PR**: Commit 0363d67 is the main one that prevents #9203 from being reintroduced by ensuring that widget div only clears its contents after ephemeral focus has returned. This was missed in the first audit since it wasn't clear that this line, in particular, can cause a div with focus to be removed and thus focus lost: https://github.com/google/blockly/blob/dfd565957b7652929ac07f4f7330390dfc459e94/core/widgetdiv.ts#L156 ### Test Coverage New tests have been added for both the drop-down div and `FocusManager` components, and have been verified as failing without the new behaviors in place. There may be other edge cases worth testing for `FocusManager` in particular, but the tests introduced in this PR seem to cover the most important cases. **New in v2 of this PR**: A test was added to validate that widget div now clears its contents only after ephemeral focus has returned to avoid the desyncing scenario that led to #9203. This test has been verified to fail without the fix. There are also a few new tests being added in the keyboard navigation plugin repository that also validate this behavior at a higher level (see RaspberryPiFoundation/blockly-keyboard-experimentation#649). Demonstration of the new behavior: [Screen recording 2025-07-01 6.28.37 PM.webm](https://github.com/user-attachments/assets/7af29fed-1ba1-4828-a6cd-65bb94509e72) ### Documentation No new documentation changes seem needed beyond the code documentation updates. ### Additional Information It's also possible to change the automatic restoration behavior to be conditional instead of always assuming focus shouldn't be reset if focus leaves the ephemeral element, but that's probably a better change if there's an actual user issue discovered with this approach. This was originally introduced in #9175 but it was reverted in #9204 due to #9203.
1 parent e3d17be commit 0e16b04

File tree

7 files changed

+392
-20
lines changed

7 files changed

+392
-20
lines changed

core/dropdowndiv.ts

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,8 @@ export function setColour(backgroundColour: string, borderColour: string) {
213213
* passed in here then callers should manage ephemeral focus directly
214214
* otherwise focus may not properly restore when the widget closes. Defaults
215215
* to true.
216+
* @param autoCloseOnLostFocus Whether the drop-down should automatically hide
217+
* if it loses DOM focus for any reason.
216218
* @returns True if the menu rendered below block; false if above.
217219
*/
218220
export function showPositionedByBlock<T>(
@@ -221,11 +223,13 @@ export function showPositionedByBlock<T>(
221223
opt_onHide?: () => void,
222224
opt_secondaryYOffset?: number,
223225
manageEphemeralFocus: boolean = true,
226+
autoCloseOnLostFocus: boolean = true,
224227
): boolean {
225228
return showPositionedByRect(
226229
getScaledBboxOfBlock(block),
227230
field as Field,
228231
manageEphemeralFocus,
232+
autoCloseOnLostFocus,
229233
opt_onHide,
230234
opt_secondaryYOffset,
231235
);
@@ -245,19 +249,23 @@ export function showPositionedByBlock<T>(
245249
* passed in here then callers should manage ephemeral focus directly
246250
* otherwise focus may not properly restore when the widget closes. Defaults
247251
* to true.
252+
* @param autoCloseOnLostFocus Whether the drop-down should automatically hide
253+
* if it loses DOM focus for any reason.
248254
* @returns True if the menu rendered below block; false if above.
249255
*/
250256
export function showPositionedByField<T>(
251257
field: Field<T>,
252258
opt_onHide?: () => void,
253259
opt_secondaryYOffset?: number,
254260
manageEphemeralFocus: boolean = true,
261+
autoCloseOnLostFocus: boolean = true,
255262
): boolean {
256263
positionToField = true;
257264
return showPositionedByRect(
258265
getScaledBboxOfField(field as Field),
259266
field as Field,
260267
manageEphemeralFocus,
268+
autoCloseOnLostFocus,
261269
opt_onHide,
262270
opt_secondaryYOffset,
263271
);
@@ -302,12 +310,15 @@ function getScaledBboxOfField(field: Field): Rect {
302310
* according to the drop-down div's lifetime. Note that if a false value is
303311
* passed in here then callers should manage ephemeral focus directly
304312
* otherwise focus may not properly restore when the widget closes.
313+
* @param autoCloseOnLostFocus Whether the drop-down should automatically hide
314+
* if it loses DOM focus for any reason.
305315
* @returns True if the menu rendered below block; false if above.
306316
*/
307317
function showPositionedByRect(
308318
bBox: Rect,
309319
field: Field,
310320
manageEphemeralFocus: boolean,
321+
autoCloseOnLostFocus: boolean,
311322
opt_onHide?: () => void,
312323
opt_secondaryYOffset?: number,
313324
): boolean {
@@ -335,6 +346,7 @@ function showPositionedByRect(
335346
secondaryX,
336347
secondaryY,
337348
manageEphemeralFocus,
349+
autoCloseOnLostFocus,
338350
opt_onHide,
339351
);
340352
}
@@ -357,6 +369,8 @@ function showPositionedByRect(
357369
* @param opt_onHide Optional callback for when the drop-down is hidden.
358370
* @param manageEphemeralFocus Whether ephemeral focus should be managed
359371
* according to the widget div's lifetime.
372+
* @param autoCloseOnLostFocus Whether the drop-down should automatically hide
373+
* if it loses DOM focus for any reason.
360374
* @returns True if the menu rendered at the primary origin point.
361375
* @internal
362376
*/
@@ -368,6 +382,7 @@ export function show<T>(
368382
secondaryX: number,
369383
secondaryY: number,
370384
manageEphemeralFocus: boolean,
385+
autoCloseOnLostFocus: boolean,
371386
opt_onHide?: () => void,
372387
): boolean {
373388
owner = newOwner as Field;
@@ -394,7 +409,18 @@ export function show<T>(
394409
// Ephemeral focus must happen after the div is fully visible in order to
395410
// ensure that it properly receives focus.
396411
if (manageEphemeralFocus) {
397-
returnEphemeralFocus = getFocusManager().takeEphemeralFocus(div);
412+
const autoCloseCallback = autoCloseOnLostFocus
413+
? (hasFocus: boolean) => {
414+
// If focus is ever lost, close the drop-down.
415+
if (!hasFocus) {
416+
hide();
417+
}
418+
}
419+
: null;
420+
returnEphemeralFocus = getFocusManager().takeEphemeralFocus(
421+
div,
422+
autoCloseCallback,
423+
);
398424
}
399425

400426
return atOrigin;
@@ -693,7 +719,6 @@ export function hideWithoutAnimation() {
693719
onHide();
694720
onHide = null;
695721
}
696-
clearContent();
697722
owner = null;
698723

699724
(common.getMainWorkspace() as WorkspaceSvg).markFocused();
@@ -702,6 +727,13 @@ export function hideWithoutAnimation() {
702727
returnEphemeralFocus();
703728
returnEphemeralFocus = null;
704729
}
730+
731+
// Content must be cleared after returning ephemeral focus since otherwise it
732+
// may force focus changes which could desynchronize the focus manager and
733+
// make it think the user directed focus away from the drop-down div (which
734+
// will then notify it to not restore focus back to any previously focused
735+
// node).
736+
clearContent();
705737
}
706738

707739
/**

core/focus_manager.ts

Lines changed: 80 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,14 @@ import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js';
1717
*/
1818
export type ReturnEphemeralFocus = () => void;
1919

20+
/**
21+
* Type declaration for an optional callback to observe when an element with
22+
* ephemeral focus has its DOM focus changed before ephemeral focus is returned.
23+
*
24+
* See FocusManager.takeEphemeralFocus for more details.
25+
*/
26+
export type EphemeralFocusChangedInDom = (hasDomFocus: boolean) => void;
27+
2028
/**
2129
* Represents an IFocusableTree that has been registered for focus management in
2230
* FocusManager.
@@ -78,7 +86,10 @@ export class FocusManager {
7886
private previouslyFocusedNode: IFocusableNode | null = null;
7987
private registeredTrees: Array<TreeRegistration> = [];
8088

81-
private currentlyHoldsEphemeralFocus: boolean = false;
89+
private ephemerallyFocusedElement: HTMLElement | SVGElement | null = null;
90+
private ephemeralDomFocusChangedCallback: EphemeralFocusChangedInDom | null =
91+
null;
92+
private ephemerallyFocusedElementCurrentlyHasFocus: boolean = false;
8293
private lockFocusStateChanges: boolean = false;
8394
private recentlyLostAllFocus: boolean = false;
8495
private isUpdatingFocusedNode: boolean = false;
@@ -118,6 +129,21 @@ export class FocusManager {
118129
} else {
119130
this.defocusCurrentFocusedNode();
120131
}
132+
133+
const ephemeralFocusElem = this.ephemerallyFocusedElement;
134+
if (ephemeralFocusElem) {
135+
const hadFocus = this.ephemerallyFocusedElementCurrentlyHasFocus;
136+
const hasFocus =
137+
!!element &&
138+
element instanceof Node &&
139+
ephemeralFocusElem.contains(element);
140+
if (hadFocus !== hasFocus) {
141+
if (this.ephemeralDomFocusChangedCallback) {
142+
this.ephemeralDomFocusChangedCallback(hasFocus);
143+
}
144+
this.ephemerallyFocusedElementCurrentlyHasFocus = hasFocus;
145+
}
146+
}
121147
};
122148

123149
// Register root document focus listeners for tracking when focus leaves all
@@ -313,7 +339,7 @@ export class FocusManager {
313339
*/
314340
focusNode(focusableNode: IFocusableNode): void {
315341
this.ensureManagerIsUnlocked();
316-
const mustRestoreUpdatingNode = !this.currentlyHoldsEphemeralFocus;
342+
const mustRestoreUpdatingNode = !this.ephemerallyFocusedElement;
317343
if (mustRestoreUpdatingNode) {
318344
// Disable state syncing from DOM events since possible calls to focus()
319345
// below will loop a call back to focusNode().
@@ -395,7 +421,7 @@ export class FocusManager {
395421
this.removeHighlight(nextTreeRoot);
396422
}
397423

398-
if (!this.currentlyHoldsEphemeralFocus) {
424+
if (!this.ephemerallyFocusedElement) {
399425
// Only change the actively focused node if ephemeral state isn't held.
400426
this.activelyFocusNode(nodeToFocus, prevTree ?? null);
401427
}
@@ -423,24 +449,50 @@ export class FocusManager {
423449
* the returned lambda is called. Additionally, only 1 ephemeral focus context
424450
* can be active at any given time (attempting to activate more than one
425451
* simultaneously will result in an error being thrown).
452+
*
453+
* Important details regarding the onFocusChangedInDom callback:
454+
* - This method will be called initially with a value of 'true' indicating
455+
* that the ephemeral element has been focused, so callers can rely on that,
456+
* if needed, for initialization logic.
457+
* - It's safe to end ephemeral focus in this callback (and is encouraged for
458+
* callers that wish to automatically end ephemeral focus when the user
459+
* directs focus outside of the element).
460+
* - The element AND all of its descendants are tracked for focus. That means
461+
* the callback will ONLY be called with a value of 'false' if focus
462+
* completely leaves the DOM tree for the provided focusable element.
463+
* - It's invalid to return focus on the very first call to the callback,
464+
* however this is expected to be impossible, anyway, since this method
465+
* won't return until after the first call to the callback (thus there will
466+
* be no means to return ephemeral focus).
467+
*
468+
* @param focusableElement The element that should be focused until returned.
469+
* @param onFocusChangedInDom An optional callback which will be notified
470+
* whenever the provided element's focus changes before ephemeral focus is
471+
* returned. See the details above for specifics.
472+
* @returns A ReturnEphemeralFocus that must be called when ephemeral focus
473+
* should end.
426474
*/
427475
takeEphemeralFocus(
428476
focusableElement: HTMLElement | SVGElement,
477+
onFocusChangedInDom: EphemeralFocusChangedInDom | null = null,
429478
): ReturnEphemeralFocus {
430479
this.ensureManagerIsUnlocked();
431-
if (this.currentlyHoldsEphemeralFocus) {
480+
if (this.ephemerallyFocusedElement) {
432481
throw Error(
433482
`Attempted to take ephemeral focus when it's already held, ` +
434483
`with new element: ${focusableElement}.`,
435484
);
436485
}
437-
this.currentlyHoldsEphemeralFocus = true;
486+
this.ephemerallyFocusedElement = focusableElement;
487+
this.ephemeralDomFocusChangedCallback = onFocusChangedInDom;
438488

439489
if (this.focusedNode) {
440490
this.passivelyFocusNode(this.focusedNode, null);
441491
}
442492
focusableElement.focus();
493+
this.ephemerallyFocusedElementCurrentlyHasFocus = true;
443494

495+
const focusedNodeAtStart = this.focusedNode;
444496
let hasFinishedEphemeralFocus = false;
445497
return () => {
446498
if (hasFinishedEphemeralFocus) {
@@ -450,9 +502,22 @@ export class FocusManager {
450502
);
451503
}
452504
hasFinishedEphemeralFocus = true;
453-
this.currentlyHoldsEphemeralFocus = false;
454-
455-
if (this.focusedNode) {
505+
this.ephemerallyFocusedElement = null;
506+
this.ephemeralDomFocusChangedCallback = null;
507+
508+
const hadEphemeralFocusAtEnd =
509+
this.ephemerallyFocusedElementCurrentlyHasFocus;
510+
this.ephemerallyFocusedElementCurrentlyHasFocus = false;
511+
512+
// If the user forced away DOM focus during ephemeral focus, then
513+
// determine whether focus should be restored back to a focusable node
514+
// after ephemeral focus ends. Generally it shouldn't be, but in some
515+
// cases (such as the user focusing an actual focusable node) it then
516+
// should be.
517+
const hasNewFocusedNode = focusedNodeAtStart !== this.focusedNode;
518+
const shouldRestoreToNode = hasNewFocusedNode || hadEphemeralFocusAtEnd;
519+
520+
if (this.focusedNode && shouldRestoreToNode) {
456521
this.activelyFocusNode(this.focusedNode, null);
457522

458523
// Even though focus was restored, check if it's lost again. It's
@@ -470,6 +535,11 @@ export class FocusManager {
470535
this.focusNode(capturedNode);
471536
}
472537
}, 0);
538+
} else {
539+
// If the ephemeral element lost focus then do not force it back since
540+
// that likely will override the user's own attempt to move focus away
541+
// from the ephemeral experience.
542+
this.defocusCurrentFocusedNode();
473543
}
474544
};
475545
}
@@ -478,7 +548,7 @@ export class FocusManager {
478548
* @returns whether something is currently holding ephemeral focus
479549
*/
480550
ephemeralFocusTaken(): boolean {
481-
return this.currentlyHoldsEphemeralFocus;
551+
return !!this.ephemerallyFocusedElement;
482552
}
483553

484554
/**
@@ -516,7 +586,7 @@ export class FocusManager {
516586
// The current node will likely be defocused while ephemeral focus is held,
517587
// but internal manager state shouldn't change since the node should be
518588
// restored upon exiting ephemeral focus mode.
519-
if (this.focusedNode && !this.currentlyHoldsEphemeralFocus) {
589+
if (this.focusedNode && !this.ephemerallyFocusedElement) {
520590
this.passivelyFocusNode(this.focusedNode, null);
521591
this.updateFocusedNode(null);
522592
}

core/widgetdiv.ts

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -146,6 +146,18 @@ export function hide() {
146146

147147
const div = containerDiv;
148148
if (!div) return;
149+
150+
(common.getMainWorkspace() as WorkspaceSvg).markFocused();
151+
152+
if (returnEphemeralFocus) {
153+
returnEphemeralFocus();
154+
returnEphemeralFocus = null;
155+
}
156+
157+
// Content must be cleared after returning ephemeral focus since otherwise it
158+
// may force focus changes which could desynchronize the focus manager and
159+
// make it think the user directed focus away from the widget div (which will
160+
// then notify it to not restore focus back to any previously focused node).
149161
div.style.display = 'none';
150162
div.style.left = '';
151163
div.style.top = '';
@@ -163,12 +175,6 @@ export function hide() {
163175
dom.removeClass(div, themeClassName);
164176
themeClassName = '';
165177
}
166-
(common.getMainWorkspace() as WorkspaceSvg).markFocused();
167-
168-
if (returnEphemeralFocus) {
169-
returnEphemeralFocus();
170-
returnEphemeralFocus = null;
171-
}
172178
}
173179

174180
/**

0 commit comments

Comments
 (0)