From 40485761ad55d86e61fb67a4b8370e590330ffc4 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 1 May 2025 10:47:33 -0700 Subject: [PATCH 1/6] fix: set focus from clicks --- core/gesture.ts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/core/gesture.ts b/core/gesture.ts index fc23ba7ca15..7b75b5e0239 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -25,6 +25,7 @@ import * as dropDownDiv from './dropdowndiv.js'; import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; import type {Field} from './field.js'; +import {getFocusManager} from './focus_manager.js'; import type {IBubble} from './interfaces/i_bubble.js'; import {IDraggable, isDraggable} from './interfaces/i_draggable.js'; import {IDragger} from './interfaces/i_dragger.js'; @@ -289,7 +290,7 @@ export class Gesture { // The start block is no longer relevant, because this is a drag. this.startBlock = null; this.targetBlock = this.flyout.createBlock(this.targetBlock); - common.setSelected(this.targetBlock); + getFocusManager().focusNode(this.targetBlock); return true; } return false; @@ -726,6 +727,7 @@ export class Gesture { if (this.targetBlock) { this.bringBlockToFront(); this.targetBlock.workspace.hideChaff(!!this.flyout); + getFocusManager().focusNode(this.targetBlock); this.targetBlock.showContextMenu(e); } else if (this.startBubble) { this.startBubble.showContextMenu(e); @@ -734,6 +736,7 @@ export class Gesture { this.startComment.showContextMenu(e); } else if (this.startWorkspace_ && !this.flyout) { this.startWorkspace_.hideChaff(); + getFocusManager().focusNode(this.startWorkspace_); this.startWorkspace_.showContextMenu(e); } @@ -762,9 +765,10 @@ export class Gesture { this.mostRecentEvent = e; if (!this.startBlock && !this.startBubble && !this.startComment) { - // Selection determines what things start drags. So to drag the workspace, - // we need to deselect anything that was previously selected. - common.setSelected(null); + // Ensure the workspace is selected if nothing else should be. + getFocusManager().focusNode(ws); + } else if (this.startBlock) { + getFocusManager().focusNode(this.startBlock); } this.doStart(e); @@ -901,6 +905,7 @@ export class Gesture { const newBlock = this.flyout.createBlock(this.targetBlock); newBlock.snapToGrid(); newBlock.bumpNeighbours(); + getFocusManager().focusNode(newBlock); } } else { if (!this.startWorkspace_) { @@ -916,6 +921,9 @@ export class Gesture { 'block', ); eventUtils.fire(event); + if (this.targetBlock) { + getFocusManager().focusNode(this.targetBlock); + } } this.bringBlockToFront(); eventUtils.setGroup(false); @@ -1023,7 +1031,6 @@ export class Gesture { // If the gesture already went through a bubble, don't set the start block. if (!this.startBlock && !this.startBubble) { this.startBlock = block; - common.setSelected(this.startBlock); if (block.isInFlyout && block !== block.getRootBlock()) { this.setTargetBlock(block.getRootBlock()); } else { From 7449d87fa6c670d6d87c6b5bd8fefed5ad3affb2 Mon Sep 17 00:00:00 2001 From: Rachel Fenichel Date: Thu, 1 May 2025 11:06:49 -0700 Subject: [PATCH 2/6] fix: remove outline on focused nodes and reselect blocks --- core/gesture.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/core/gesture.ts b/core/gesture.ts index 7b75b5e0239..158046e56d7 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -291,6 +291,7 @@ export class Gesture { this.startBlock = null; this.targetBlock = this.flyout.createBlock(this.targetBlock); getFocusManager().focusNode(this.targetBlock); + common.setSelected(this.targetBlock); return true; } return false; @@ -767,6 +768,7 @@ export class Gesture { if (!this.startBlock && !this.startBubble && !this.startComment) { // Ensure the workspace is selected if nothing else should be. getFocusManager().focusNode(ws); + common.setSelected(null); } else if (this.startBlock) { getFocusManager().focusNode(this.startBlock); } @@ -1031,6 +1033,7 @@ export class Gesture { // If the gesture already went through a bubble, don't set the start block. if (!this.startBlock && !this.startBubble) { this.startBlock = block; + common.setSelected(this.startBlock); if (block.isInFlyout && block !== block.getRootBlock()) { this.setTargetBlock(block.getRootBlock()); } else { From 8bf37ac027eb7f735a046e5cce72d7d7c27033a9 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 22:47:44 +0000 Subject: [PATCH 3/6] fix: Ensure gestures & focus synchronize. These fixes cover a number of specific cases, plus some additional safety improvements were added for widget and drop-down divs, plus ephemeral focus management. --- core/block_svg.ts | 9 ++++++++ core/clipboard/block_paster.ts | 9 ++++++-- core/dragging/dragger.ts | 8 +++++++ core/dropdowndiv.ts | 13 +++++------ core/field_dropdown.ts | 4 ++-- core/focus_manager.ts | 40 +++++++++++++++++++++++++++++----- core/gesture.ts | 29 ++++++++++++------------ core/widgetdiv.ts | 9 ++++---- 8 files changed, 86 insertions(+), 35 deletions(-) diff --git a/core/block_svg.ts b/core/block_svg.ts index 151ee868fea..56a4dc7eaea 100644 --- a/core/block_svg.ts +++ b/core/block_svg.ts @@ -34,6 +34,7 @@ import type {BlockMove} from './events/events_block_move.js'; import {EventType} from './events/type.js'; import * as eventUtils from './events/utils.js'; import {FieldLabel} from './field_label.js'; +import {getFocusManager} from './focus_manager.js'; import {IconType} from './icons/icon_types.js'; import {MutatorIcon} from './icons/mutator_icon.js'; import {WarningIcon} from './icons/warning_icon.js'; @@ -1268,6 +1269,7 @@ export class BlockSvg * adjusting its parents. */ bringToFront(blockOnly = false) { + const previouslyFocused = getFocusManager().getFocusedNode(); /* eslint-disable-next-line @typescript-eslint/no-this-alias */ let block: this | null = this; if (block.isDeadOrDying()) { @@ -1284,6 +1286,13 @@ export class BlockSvg if (blockOnly) break; block = block.getParent(); } while (block); + if (previouslyFocused) { + // Bringing a block to the front of the stack doesn't fundamentally change + // the logical structure of the page, but it does change element ordering + // which can take automatically take away focus from a node. Ensure focus + // is restored to avoid a discontinuity. + getFocusManager().focusNode(previouslyFocused); + } } /** diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index 08ff220ee91..67a17dfdc8c 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -5,10 +5,11 @@ */ import {BlockSvg} from '../block_svg.js'; -import * as common from '../common.js'; +import {IFocusableNode} from '../blockly.js'; import {config} from '../config.js'; import {EventType} from '../events/type.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import {ICopyData} from '../interfaces/i_copyable.js'; import {IPaster} from '../interfaces/i_paster.js'; import {State, append} from '../serialization/blocks.js'; @@ -55,7 +56,11 @@ export class BlockPaster implements IPaster { if (eventUtils.isEnabled() && !block.isShadow()) { eventUtils.fire(new (eventUtils.get(EventType.BLOCK_CREATE))(block)); } - common.setSelected(block); + + // Sometimes there's a delay before the block is fully created and ready for + // focusing, so wait slightly before focusing the newly pasted block. + const nodeToFocus: IFocusableNode = block; + setTimeout(() => getFocusManager().focusNode(nodeToFocus), 1); return block; } } diff --git a/core/dragging/dragger.ts b/core/dragging/dragger.ts index 518351d5c86..36a813e0a19 100644 --- a/core/dragging/dragger.ts +++ b/core/dragging/dragger.ts @@ -8,11 +8,13 @@ import * as blockAnimations from '../block_animations.js'; import {BlockSvg} from '../block_svg.js'; import {ComponentManager} from '../component_manager.js'; import * as eventUtils from '../events/utils.js'; +import {getFocusManager} from '../focus_manager.js'; import {IDeletable, isDeletable} from '../interfaces/i_deletable.js'; import {IDeleteArea} from '../interfaces/i_delete_area.js'; import {IDragTarget} from '../interfaces/i_drag_target.js'; import {IDraggable} from '../interfaces/i_draggable.js'; import {IDragger} from '../interfaces/i_dragger.js'; +import {isFocusableNode} from '../interfaces/i_focusable_node.js'; import * as registry from '../registry.js'; import {Coordinate} from '../utils/coordinate.js'; import {WorkspaceSvg} from '../workspace_svg.js'; @@ -129,6 +131,12 @@ export class Dragger implements IDragger { root.dispose(); } eventUtils.setGroup(false); + + if (!wouldDelete && isFocusableNode(this.draggable)) { + // Ensure focusable nodes that have finished dragging (but aren't being) + // deleted end with focus and selection. + getFocusManager().focusNode(this.draggable); + } } // We need to special case blocks for now so that we look at the root block diff --git a/core/dropdowndiv.ts b/core/dropdowndiv.ts index dcf8fa24ef7..e326ac94cb4 100644 --- a/core/dropdowndiv.ts +++ b/core/dropdowndiv.ts @@ -629,10 +629,6 @@ export function hide() { animateOutTimer = setTimeout(function () { hideWithoutAnimation(); }, ANIMATION_TIME * 1000); - if (returnEphemeralFocus) { - returnEphemeralFocus(); - returnEphemeralFocus = null; - } if (onHide) { onHide(); onHide = null; @@ -648,10 +644,6 @@ export function hideWithoutAnimation() { clearTimeout(animateOutTimer); } - if (returnEphemeralFocus) { - returnEphemeralFocus(); - returnEphemeralFocus = null; - } if (onHide) { onHide(); onHide = null; @@ -660,6 +652,11 @@ export function hideWithoutAnimation() { owner = null; (common.getMainWorkspace() as WorkspaceSvg).markFocused(); + + if (returnEphemeralFocus) { + returnEphemeralFocus(); + returnEphemeralFocus = null; + } } /** diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 81279e2a1f5..20fc6820785 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -769,7 +769,7 @@ export class FieldDropdown extends Field { } else if (typeof option[1] !== 'string') { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option id must be a string. + `Invalid option[${i}]: Each FieldDropdown option id must be a string. Found ${option[1]} in: ${option}`, ); } else if ( @@ -780,7 +780,7 @@ export class FieldDropdown extends Field { ) { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option must have a string + `Invalid option[${i}]: Each FieldDropdown option must have a string label, image description, or HTML element. Found ${option[0]} in: ${option}`, ); } diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 7091c4efb08..f9a62afecbb 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -56,17 +56,20 @@ export class FocusManager { */ static readonly PASSIVE_FOCUS_NODE_CSS_CLASS_NAME = 'blocklyPassiveFocus'; - focusedNode: IFocusableNode | null = null; - registeredTrees: Array = []; + private focusedNode: IFocusableNode | null = null; + private previouslyFocusedNode: IFocusableNode | null = null; + private registeredTrees: Array = []; private currentlyHoldsEphemeralFocus: boolean = false; private lockFocusStateChanges: boolean = false; + private recentlyLostAllFocus: boolean = false; constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, ) { // Note that 'element' here is the element *gaining* focus. const maybeFocus = (element: Element | EventTarget | null) => { + this.recentlyLostAllFocus = !element; let newNode: IFocusableNode | null | undefined = null; if (element instanceof HTMLElement || element instanceof SVGElement) { // If the target losing or gaining focus maps to any tree, then it @@ -164,7 +167,7 @@ export class FocusManager { const root = tree.getRootFocusableNode(); if (focusedNode) this.removeHighlight(focusedNode); if (this.focusedNode === focusedNode || this.focusedNode === root) { - this.focusedNode = null; + this.updateFocusedNode(null); } this.removeHighlight(root); } @@ -277,7 +280,7 @@ export class FocusManager { // Only change the actively focused node if ephemeral state isn't held. this.activelyFocusNode(focusableNode, prevTree ?? null); } - this.focusedNode = focusableNode; + this.updateFocusedNode(focusableNode); } /** @@ -328,6 +331,22 @@ export class FocusManager { if (this.focusedNode) { this.activelyFocusNode(this.focusedNode, null); + + // Even though focus was restored, check if it's lost again. It's + // possible for the browser to force focus away from all elements once + // the ephemeral element disappears. This ensures focus is restored. + const capturedNode = this.focusedNode; + setTimeout(() => { + // These checks are set up to minimize the risk that a legitimate + // focus change occurred within the delay that this would override. + if ( + !this.focusedNode && + this.previouslyFocusedNode === capturedNode && + this.recentlyLostAllFocus + ) { + this.focusNode(capturedNode); + } + }, 0); } }; } @@ -348,6 +367,17 @@ export class FocusManager { } } + /** + * Updates the internally tracked focused node to the specified node, or null + * if focus is being lost. This also updates previous focus tracking. + * + * @param newFocusedNode The new node to set as focused. + */ + private updateFocusedNode(newFocusedNode: IFocusableNode | null) { + this.previouslyFocusedNode = this.focusedNode; + this.focusedNode = newFocusedNode; + } + /** * Defocuses the current actively focused node tracked by the manager, iff * there's a node being tracked and the manager doesn't have ephemeral focus. @@ -358,7 +388,7 @@ export class FocusManager { // restored upon exiting ephemeral focus mode. if (this.focusedNode && !this.currentlyHoldsEphemeralFocus) { this.passivelyFocusNode(this.focusedNode, null); - this.focusedNode = null; + this.updateFocusedNode(null); } } diff --git a/core/gesture.ts b/core/gesture.ts index 158046e56d7..f9b435c67d9 100644 --- a/core/gesture.ts +++ b/core/gesture.ts @@ -291,7 +291,6 @@ export class Gesture { this.startBlock = null; this.targetBlock = this.flyout.createBlock(this.targetBlock); getFocusManager().focusNode(this.targetBlock); - common.setSelected(this.targetBlock); return true; } return false; @@ -728,7 +727,6 @@ export class Gesture { if (this.targetBlock) { this.bringBlockToFront(); this.targetBlock.workspace.hideChaff(!!this.flyout); - getFocusManager().focusNode(this.targetBlock); this.targetBlock.showContextMenu(e); } else if (this.startBubble) { this.startBubble.showContextMenu(e); @@ -766,9 +764,10 @@ export class Gesture { this.mostRecentEvent = e; if (!this.startBlock && !this.startBubble && !this.startComment) { - // Ensure the workspace is selected if nothing else should be. + // Ensure the workspace is selected if nothing else should be. Note that + // this is focusNode() instead of focusTree() because if any active node + // is focused in the workspace it should be defocused. getFocusManager().focusNode(ws); - common.setSelected(null); } else if (this.startBlock) { getFocusManager().focusNode(this.startBlock); } @@ -871,13 +870,18 @@ export class Gesture { ); } + // Note that the order is important here: bringing a block to the front will + // cause it to become focused and showing the field editor will capture + // focus ephemerally. It's important to ensure that focus is properly + // restored back to the block after field editing has completed. + this.bringBlockToFront(); + // Only show the editor if the field's editor wasn't already open // right before this gesture started. const dropdownAlreadyOpen = this.currentDropdownOwner === this.startField; if (!dropdownAlreadyOpen) { this.startField.showEditor(this.mostRecentEvent); } - this.bringBlockToFront(); } /** Execute an icon click. */ @@ -907,6 +911,8 @@ export class Gesture { const newBlock = this.flyout.createBlock(this.targetBlock); newBlock.snapToGrid(); newBlock.bumpNeighbours(); + + // If a new block was added, make sure that it's correctly focused. getFocusManager().focusNode(newBlock); } } else { @@ -923,9 +929,6 @@ export class Gesture { 'block', ); eventUtils.fire(event); - if (this.targetBlock) { - getFocusManager().focusNode(this.targetBlock); - } } this.bringBlockToFront(); eventUtils.setGroup(false); @@ -938,11 +941,7 @@ export class Gesture { * @param _e A pointerup event. */ private doWorkspaceClick(_e: PointerEvent) { - const ws = this.creatorWorkspace; - if (common.getSelected()) { - common.getSelected()!.unselect(); - } - this.fireWorkspaceClick(this.startWorkspace_ || ws); + this.fireWorkspaceClick(this.startWorkspace_ || this.creatorWorkspace); } /* End functions defining what actions to take to execute clicks on each type @@ -957,6 +956,8 @@ export class Gesture { private bringBlockToFront() { // Blocks in the flyout don't overlap, so skip the work. if (this.targetBlock && !this.flyout) { + // Always ensure the block being dragged/clicked has focus. + getFocusManager().focusNode(this.targetBlock); this.targetBlock.bringToFront(); } } @@ -1033,7 +1034,6 @@ export class Gesture { // If the gesture already went through a bubble, don't set the start block. if (!this.startBlock && !this.startBubble) { this.startBlock = block; - common.setSelected(this.startBlock); if (block.isInFlyout && block !== block.getRootBlock()) { this.setTargetBlock(block.getRootBlock()); } else { @@ -1056,6 +1056,7 @@ export class Gesture { this.setTargetBlock(block.getParent()!); } else { this.targetBlock = block; + getFocusManager().focusNode(block); } } diff --git a/core/widgetdiv.ts b/core/widgetdiv.ts index cb006160455..608927b6fb0 100644 --- a/core/widgetdiv.ts +++ b/core/widgetdiv.ts @@ -131,10 +131,6 @@ export function hide() { div.style.display = 'none'; div.style.left = ''; div.style.top = ''; - if (returnEphemeralFocus) { - returnEphemeralFocus(); - returnEphemeralFocus = null; - } if (dispose) { dispose(); dispose = null; @@ -150,6 +146,11 @@ export function hide() { themeClassName = ''; } (common.getMainWorkspace() as WorkspaceSvg).markFocused(); + + if (returnEphemeralFocus) { + returnEphemeralFocus(); + returnEphemeralFocus = null; + } } /** From 54950c4b2371574604988b81c2b0d1f6134ab010 Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 23:11:49 +0000 Subject: [PATCH 4/6] chore: revert file + try lower delay. Addresses self-review comments. --- core/clipboard/block_paster.ts | 2 +- core/field_dropdown.ts | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index 67a17dfdc8c..caacd158138 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -60,7 +60,7 @@ export class BlockPaster implements IPaster { // Sometimes there's a delay before the block is fully created and ready for // focusing, so wait slightly before focusing the newly pasted block. const nodeToFocus: IFocusableNode = block; - setTimeout(() => getFocusManager().focusNode(nodeToFocus), 1); + setTimeout(() => getFocusManager().focusNode(nodeToFocus), 0); return block; } } diff --git a/core/field_dropdown.ts b/core/field_dropdown.ts index 20fc6820785..81279e2a1f5 100644 --- a/core/field_dropdown.ts +++ b/core/field_dropdown.ts @@ -769,7 +769,7 @@ export class FieldDropdown extends Field { } else if (typeof option[1] !== 'string') { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option id must be a string. + `Invalid option[${i}]: Each FieldDropdown option id must be a string. Found ${option[1]} in: ${option}`, ); } else if ( @@ -780,7 +780,7 @@ export class FieldDropdown extends Field { ) { foundError = true; console.error( - `Invalid option[${i}]: Each FieldDropdown option must have a string + `Invalid option[${i}]: Each FieldDropdown option must have a string label, image description, or HTML element. Found ${option[0]} in: ${option}`, ); } From 646e943ced41c2407837b688148c2be0fc95bbbb Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Fri, 2 May 2025 23:51:48 +0000 Subject: [PATCH 5/6] chore: fix comment Addresses reviewer comment. --- core/dragging/dragger.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/dragging/dragger.ts b/core/dragging/dragger.ts index 36a813e0a19..02e9e2bfb79 100644 --- a/core/dragging/dragger.ts +++ b/core/dragging/dragger.ts @@ -133,8 +133,8 @@ export class Dragger implements IDragger { eventUtils.setGroup(false); if (!wouldDelete && isFocusableNode(this.draggable)) { - // Ensure focusable nodes that have finished dragging (but aren't being) - // deleted end with focus and selection. + // Ensure focusable nodes that have finished dragging (but aren't being + // deleted) end with focus and selection. getFocusManager().focusNode(this.draggable); } } From 919986699927c6d6522254b12ace3cb73f843b5e Mon Sep 17 00:00:00 2001 From: Ben Henning Date: Mon, 5 May 2025 17:24:53 +0000 Subject: [PATCH 6/6] Use finishQueuedRenders instead of setTimeout. This addresses a review comment. --- core/clipboard/block_paster.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/clipboard/block_paster.ts b/core/clipboard/block_paster.ts index caacd158138..750cedca124 100644 --- a/core/clipboard/block_paster.ts +++ b/core/clipboard/block_paster.ts @@ -12,6 +12,7 @@ import * as eventUtils from '../events/utils.js'; import {getFocusManager} from '../focus_manager.js'; import {ICopyData} from '../interfaces/i_copyable.js'; import {IPaster} from '../interfaces/i_paster.js'; +import * as renderManagement from '../render_management.js'; import {State, append} from '../serialization/blocks.js'; import {Coordinate} from '../utils/coordinate.js'; import {WorkspaceSvg} from '../workspace_svg.js'; @@ -60,7 +61,9 @@ export class BlockPaster implements IPaster { // Sometimes there's a delay before the block is fully created and ready for // focusing, so wait slightly before focusing the newly pasted block. const nodeToFocus: IFocusableNode = block; - setTimeout(() => getFocusManager().focusNode(nodeToFocus), 0); + renderManagement + .finishQueuedRenders() + .then(() => getFocusManager().focusNode(nodeToFocus)); return block; } }