diff --git a/core/blockly.ts b/core/blockly.ts index c38a1d48e4b..069e21c81f6 100644 --- a/core/blockly.ts +++ b/core/blockly.ts @@ -120,6 +120,7 @@ import * as inputs from './inputs.js'; import {IFlyoutInflater} from './interfaces/i_flyout_inflater.js'; import {LabelFlyoutInflater} from './label_flyout_inflater.js'; import {SeparatorFlyoutInflater} from './separator_flyout_inflater.js'; +import {FocusableTreeTraverser} from './utils/focusable_tree_traverser.js'; import {Input} from './inputs/input.js'; import {InsertionMarkerPreviewer} from './insertion_marker_previewer.js'; @@ -527,6 +528,7 @@ export { FlyoutMetricsManager, FlyoutSeparator, FocusManager, + FocusableTreeTraverser, CodeGenerator as Generator, Gesture, Grid, diff --git a/core/focus_manager.ts b/core/focus_manager.ts index 88eef46b530..7091c4efb08 100644 --- a/core/focus_manager.ts +++ b/core/focus_manager.ts @@ -65,26 +65,15 @@ export class FocusManager { constructor( addGlobalEventListener: (type: string, listener: EventListener) => void, ) { - // Register root document focus listeners for tracking when focus leaves all - // tracked focusable trees. - addGlobalEventListener('focusin', (event) => { - if (!(event instanceof FocusEvent)) return; - - // The target that now has focus. - const activeElement = document.activeElement; + // Note that 'element' here is the element *gaining* focus. + const maybeFocus = (element: Element | EventTarget | null) => { let newNode: IFocusableNode | null | undefined = null; - if ( - activeElement instanceof HTMLElement || - activeElement instanceof SVGElement - ) { - // If the target losing focus maps to any tree, then it should be - // updated. Per the contract of findFocusableNodeFor only one tree - // should claim the element. + if (element instanceof HTMLElement || element instanceof SVGElement) { + // If the target losing or gaining focus maps to any tree, then it + // should be updated. Per the contract of findFocusableNodeFor only one + // tree should claim the element, so the search can be exited early. for (const tree of this.registeredTrees) { - newNode = FocusableTreeTraverser.findFocusableNodeFor( - activeElement, - tree, - ); + newNode = FocusableTreeTraverser.findFocusableNodeFor(element, tree); if (newNode) break; } } @@ -103,6 +92,26 @@ export class FocusManager { } else { this.defocusCurrentFocusedNode(); } + }; + + // Register root document focus listeners for tracking when focus leaves all + // tracked focusable trees. Note that focusin and focusout can be somewhat + // overlapping in the information that they provide. This is fine because + // they both aim to check for focus changes on the element gaining or having + // received focus, and maybeFocus should behave relatively deterministic. + addGlobalEventListener('focusin', (event) => { + if (!(event instanceof FocusEvent)) return; + + // When something receives focus, always use the current active element as + // the source of truth. + maybeFocus(document.activeElement); + }); + addGlobalEventListener('focusout', (event) => { + if (!(event instanceof FocusEvent)) return; + + // When something loses focus, it seems that document.activeElement may + // not necessarily be correct. Instead, use relatedTarget. + maybeFocus(event.relatedTarget); }); } @@ -247,7 +256,7 @@ export class FocusManager { const prevNode = this.focusedNode; const prevTree = prevNode?.getFocusableTree(); - if (prevNode && prevTree !== nextTree) { + if (prevNode) { this.passivelyFocusNode(prevNode, nextTree); } @@ -374,7 +383,9 @@ export class FocusManager { // node's focusable element (which *is* allowed to be invisible until the // node needs to be focused). this.lockFocusStateChanges = true; - node.getFocusableTree().onTreeFocus(node, prevTree); + if (node.getFocusableTree() !== prevTree) { + node.getFocusableTree().onTreeFocus(node, prevTree); + } node.onNodeFocus(); this.lockFocusStateChanges = false; @@ -399,11 +410,15 @@ export class FocusManager { nextTree: IFocusableTree | null, ): void { this.lockFocusStateChanges = true; - node.getFocusableTree().onTreeBlur(nextTree); + if (node.getFocusableTree() !== nextTree) { + node.getFocusableTree().onTreeBlur(nextTree); + } node.onNodeBlur(); this.lockFocusStateChanges = false; - this.setNodeToVisualPassiveFocus(node); + if (node.getFocusableTree() !== nextTree) { + this.setNodeToVisualPassiveFocus(node); + } } /** @@ -441,19 +456,24 @@ export class FocusManager { dom.removeClass(element, FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME); dom.removeClass(element, FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME); } -} -let focusManager: FocusManager | null = null; + private static focusManager: FocusManager | null = null; -/** - * Returns the page-global FocusManager. - * - * The returned instance is guaranteed to not change across function calls, but - * may change across page loads. - */ -export function getFocusManager(): FocusManager { - if (!focusManager) { - focusManager = new FocusManager(document.addEventListener); + /** + * Returns the page-global FocusManager. + * + * The returned instance is guaranteed to not change across function calls, but + * may change across page loads. + */ + static getFocusManager(): FocusManager { + if (!FocusManager.focusManager) { + FocusManager.focusManager = new FocusManager(document.addEventListener); + } + return FocusManager.focusManager; } - return focusManager; +} + +/** Convenience function for FocusManager.getFocusManager. */ +export function getFocusManager(): FocusManager { + return FocusManager.getFocusManager(); } diff --git a/core/keyboard_nav/line_cursor.ts b/core/keyboard_nav/line_cursor.ts index 2025da7bf06..196a0854763 100644 --- a/core/keyboard_nav/line_cursor.ts +++ b/core/keyboard_nav/line_cursor.ts @@ -18,11 +18,9 @@ import {BlockSvg} from '../block_svg.js'; import * as common from '../common.js'; import type {Connection} from '../connection.js'; import {ConnectionType} from '../connection_type.js'; -import type {Abstract} from '../events/events_abstract.js'; -import {Click, ClickTarget} from '../events/events_click.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 {isFocusableNode} from '../interfaces/i_focusable_node.js'; import * as registry from '../registry.js'; import type {MarkerSvg} from '../renderers/common/marker_svg.js'; import type {PathObject} from '../renderers/zelos/path_object.js'; @@ -70,23 +68,12 @@ export class LineCursor extends Marker { options?: Partial, ) { super(); - // Bind changeListener to facilitate future disposal. - this.changeListener = this.changeListener.bind(this); - this.workspace.addChangeListener(this.changeListener); // Regularise options and apply defaults. this.options = {...defaultOptions, ...options}; this.isZelos = workspace.getRenderer() instanceof Renderer; } - /** - * Clean up this cursor. - */ - dispose() { - this.workspace.removeChangeListener(this.changeListener); - super.dispose(); - } - /** * Moves the cursor to the next previous connection, next connection or block * in the pre order traversal. Finds the next node in the pre order traversal. @@ -331,8 +318,8 @@ export class LineCursor extends Marker { * @param node The current position in the AST. * @param isValid A function true/false depending on whether the given node * should be traversed. - * @param loop Whether to loop around to the beginning of the workspace if - * novalid node was found. + * @param loop Whether to loop around to the beginning of the workspace if no + * valid node was found. * @returns The next node in the traversal. */ getNextNode( @@ -385,8 +372,8 @@ export class LineCursor extends Marker { * @param node The current position in the AST. * @param isValid A function true/false depending on whether the given node * should be traversed. - * @param loop Whether to loop around to the end of the workspace if no - * valid node was found. + * @param loop Whether to loop around to the end of the workspace if no valid + * node was found. * @returns The previous node in the traversal or null if no previous node * exists. */ @@ -527,7 +514,12 @@ export class LineCursor extends Marker { * @returns The current field, connection, or block the cursor is on. */ override getCurNode(): ASTNode | null { - this.updateCurNodeFromSelection(); + if (!this.updateCurNodeFromFocus()) { + // Fall back to selection if focus fails to sync. This can happen for + // non-focusable nodes or for cases when focus may not properly propagate + // (such as for mouse clicks). + this.updateCurNodeFromSelection(); + } return super.getCurNode(); } @@ -572,16 +564,15 @@ export class LineCursor extends Marker { * this.drawMarker() instead of this.drawer.draw() directly. * * @param newNode The new location of the cursor. - * @param updateSelection If true (the default) we'll update the selection - * too. */ - override setCurNode(newNode: ASTNode | null, updateSelection = true) { - if (updateSelection) { - this.updateSelectionFromNode(newNode); - } - + override setCurNode(newNode: ASTNode | null) { super.setCurNode(newNode); + const newNodeLocation = newNode?.getLocation(); + if (isFocusableNode(newNodeLocation)) { + getFocusManager().focusNode(newNodeLocation); + } + // Try to scroll cursor into view. if (newNode?.getType() === ASTNode.types.BLOCK) { const block = newNode.getLocation() as BlockSvg; @@ -709,32 +700,6 @@ export class LineCursor extends Marker { } } - /** - * Event listener that syncs the cursor location to the selected block on - * SELECTED events. - * - * This does not run early enough in all cases so `getCurNode()` also updates - * the node from the selection. - * - * @param event The `Selected` event. - */ - private changeListener(event: Abstract) { - switch (event.type) { - case EventType.SELECTED: - this.updateCurNodeFromSelection(); - break; - case EventType.CLICK: { - const click = event as Click; - if ( - click.workspaceId === this.workspace.id && - click.targetType === ClickTarget.WORKSPACE - ) { - this.setCurNode(null); - } - } - } - } - /** * Updates the current node to match the selection. * @@ -749,7 +714,7 @@ export class LineCursor extends Marker { const selected = common.getSelected(); if (selected === null && curNode?.getType() === ASTNode.types.BLOCK) { - this.setCurNode(null, false); + this.setCurNode(null); return; } if (selected?.workspace !== this.workspace) { @@ -770,36 +735,35 @@ export class LineCursor extends Marker { block = block.getParent(); } if (block) { - this.setCurNode(ASTNode.createBlockNode(block), false); + this.setCurNode(ASTNode.createBlockNode(block)); } } } /** - * Updates the selection from the node. + * Updates the current node to match what's currently focused. * - * Clears the selection for non-block nodes. - * Clears the selection for shadow blocks as the selection is drawn on - * the parent but the cursor will be drawn on the shadow block itself. - * We need to take care not to later clear the current node due to that null - * selection, so we track the latest selection we're in sync with. - * - * @param newNode The new node. + * @returns Whether the current node has been set successfully from the + * current focused node. */ - private updateSelectionFromNode(newNode: ASTNode | null) { - if (newNode?.getType() === ASTNode.types.BLOCK) { - if (common.getSelected() !== newNode.getLocation()) { - eventUtils.disable(); - common.setSelected(newNode.getLocation() as BlockSvg); - eventUtils.enable(); - } - } else { - if (common.getSelected()) { - eventUtils.disable(); - common.setSelected(null); - eventUtils.enable(); + private updateCurNodeFromFocus(): boolean { + const focused = getFocusManager().getFocusedNode(); + + if (focused instanceof BlockSvg) { + const block: BlockSvg | null = focused; + if (block && block.workspace === this.workspace) { + if (block.getRootBlock() === block && this.workspace.isFlyout) { + // This block actually represents a stack. Note that this is needed + // because ASTNode special cases stack for cross-block navigation. + this.setCurNode(ASTNode.createStackNode(block)); + } else { + this.setCurNode(ASTNode.createBlockNode(block)); + } + return true; } } + + return false; } /** diff --git a/tests/mocha/cursor_test.js b/tests/mocha/cursor_test.js index 53f0714da11..55595df7b05 100644 --- a/tests/mocha/cursor_test.js +++ b/tests/mocha/cursor_test.js @@ -6,6 +6,7 @@ import {ASTNode} from '../../build/src/core/keyboard_nav/ast_node.js'; import {assert} from '../../node_modules/chai/chai.js'; +import {createRenderedBlock} from './test_helpers/block_definitions.js'; import { sharedTestSetup, sharedTestTeardown, @@ -63,11 +64,11 @@ suite('Cursor', function () { ]); this.workspace = Blockly.inject('blocklyDiv', {}); this.cursor = this.workspace.getCursor(); - const blockA = this.workspace.newBlock('input_statement'); - const blockB = this.workspace.newBlock('input_statement'); - const blockC = this.workspace.newBlock('input_statement'); - const blockD = this.workspace.newBlock('input_statement'); - const blockE = this.workspace.newBlock('field_input'); + const blockA = createRenderedBlock(this.workspace, 'input_statement'); + const blockB = createRenderedBlock(this.workspace, 'input_statement'); + const blockC = createRenderedBlock(this.workspace, 'input_statement'); + const blockD = createRenderedBlock(this.workspace, 'input_statement'); + const blockE = createRenderedBlock(this.workspace, 'field_input'); blockA.nextConnection.connect(blockB.previousConnection); blockA.inputList[0].connection.connect(blockE.outputConnection); @@ -105,12 +106,12 @@ suite('Cursor', function () { ); }); - test('In - From output connection', function () { + test('In - From attached input connection', function () { const fieldBlock = this.blocks.E; - const outputNode = ASTNode.createConnectionNode( - fieldBlock.outputConnection, + const inputConnectionNode = ASTNode.createConnectionNode( + this.blocks.A.inputList[0].connection, ); - this.cursor.setCurNode(outputNode); + this.cursor.setCurNode(inputConnectionNode); this.cursor.in(); const curNode = this.cursor.getCurNode(); assert.equal(curNode.getLocation(), fieldBlock); diff --git a/tests/mocha/focus_manager_test.js b/tests/mocha/focus_manager_test.js index 69ecfe722a5..bcd74c1a03c 100644 --- a/tests/mocha/focus_manager_test.js +++ b/tests/mocha/focus_manager_test.js @@ -71,26 +71,20 @@ suite('FocusManager', function () { const ACTIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.ACTIVE_FOCUS_NODE_CSS_CLASS_NAME}`; const PASSIVE_FOCUS_NODE_CSS_SELECTOR = `.${FocusManager.PASSIVE_FOCUS_NODE_CSS_CLASS_NAME}`; + const createFocusableTree = function (rootElementId, nestedTrees) { + return new FocusableTreeImpl( + document.getElementById(rootElementId), + nestedTrees || [], + ); + }; + const createFocusableNode = function (tree, elementId) { + return tree.addNode(document.getElementById(elementId)); + }; + setup(function () { sharedTestSetup.call(this); - const testState = this; - const addDocumentEventListener = function (type, listener) { - testState.globalDocumentEventListenerType = type; - testState.globalDocumentEventListener = listener; - document.addEventListener(type, listener); - }; - this.focusManager = new FocusManager(addDocumentEventListener); - - const createFocusableTree = function (rootElementId, nestedTrees) { - return new FocusableTreeImpl( - document.getElementById(rootElementId), - nestedTrees || [], - ); - }; - const createFocusableNode = function (tree, elementId) { - return tree.addNode(document.getElementById(elementId)); - }; + this.focusManager = getFocusManager(); this.testFocusableTree1 = createFocusableTree('testFocusableTree1'); this.testFocusableTree1Node1 = createFocusableNode( @@ -160,12 +154,6 @@ suite('FocusManager', function () { teardown(function () { sharedTestTeardown.call(this); - // Remove the globally registered listener from FocusManager to avoid state being shared across - // test boundaries. - const eventType = this.globalDocumentEventListenerType; - const eventListener = this.globalDocumentEventListener; - document.removeEventListener(eventType, eventListener); - // Ensure all node CSS styles are reset so that state isn't leaked between tests. const activeElems = document.querySelectorAll( ACTIVE_FOCUS_NODE_CSS_SELECTOR, @@ -832,6 +820,27 @@ suite('FocusManager', function () { this.testFocusableNestedTree4Node1, ); }); + + test('deletion after focusNode() returns null', function () { + const rootElem = document.createElement('div'); + const nodeElem = document.createElement('div'); + rootElem.setAttribute('id', 'focusRoot'); + rootElem.setAttribute('tabindex', '-1'); + nodeElem.setAttribute('id', 'focusNode'); + nodeElem.setAttribute('tabindex', '-1'); + nodeElem.textContent = 'Focusable node'; + rootElem.appendChild(nodeElem); + document.body.appendChild(rootElem); + const root = createFocusableTree('focusRoot'); + const node = createFocusableNode(root, 'focusNode'); + this.focusManager.registerTree(root); + this.focusManager.focusNode(node); + + node.getFocusableElement().remove(); + + assert.notStrictEqual(this.focusManager.getFocusedNode(), node); + rootElem.remove(); // Cleanup. + }); }); suite('CSS classes', function () { test('registered tree focusTree()ed no prev focus root elem has active property', function () { @@ -1724,6 +1733,27 @@ suite('FocusManager', function () { this.testFocusableNestedTree4Node1, ); }); + + test('deletion after focus() returns null', function () { + const rootElem = document.createElement('div'); + const nodeElem = document.createElement('div'); + rootElem.setAttribute('id', 'focusRoot'); + rootElem.setAttribute('tabindex', '-1'); + nodeElem.setAttribute('id', 'focusNode'); + nodeElem.setAttribute('tabindex', '-1'); + nodeElem.textContent = 'Focusable node'; + rootElem.appendChild(nodeElem); + document.body.appendChild(rootElem); + const root = createFocusableTree('focusRoot'); + const node = createFocusableNode(root, 'focusNode'); + this.focusManager.registerTree(root); + document.getElementById('focusNode').focus(); + + node.getFocusableElement().remove(); + + assert.notStrictEqual(this.focusManager.getFocusedNode(), node); + rootElem.remove(); // Cleanup. + }); }); suite('CSS classes', function () { test('registered root focus()ed no prev focus returns root elem has active property', function () { diff --git a/tests/mocha/test_helpers/setup_teardown.js b/tests/mocha/test_helpers/setup_teardown.js index 2fc08cb6943..b0d7c83c697 100644 --- a/tests/mocha/test_helpers/setup_teardown.js +++ b/tests/mocha/test_helpers/setup_teardown.js @@ -5,6 +5,7 @@ */ import * as eventUtils from '../../../build/src/core/events/utils.js'; +import {FocusManager} from '../../../build/src/core/focus_manager.js'; /** * Safely disposes of Blockly workspace, logging any errors. @@ -124,6 +125,18 @@ export function sharedTestSetup(options = {}) { }; this.blockTypesCleanup_ = this.sharedCleanup.blockTypesCleanup_; this.messagesCleanup_ = this.sharedCleanup.messagesCleanup_; + + // Set up FocusManager to run in isolation for this test. + this.globalDocumentEventListeners = []; + const testState = this; + const addDocumentEventListener = function (type, listener) { + testState.globalDocumentEventListeners.push({type, listener}); + document.addEventListener(type, listener); + }; + const specificFocusManager = new FocusManager(addDocumentEventListener); + this.oldGetFocusManager = FocusManager.getFocusManager; + FocusManager.getFocusManager = () => specificFocusManager; + wrapDefineBlocksWithJsonArrayWithCleanup_(this.sharedCleanup); return { clock: this.clock, @@ -184,6 +197,16 @@ export function sharedTestTeardown() { } Blockly.WidgetDiv.testOnly_setDiv(null); + + // Remove the globally registered listener from FocusManager to avoid state + // being shared across test boundaries. + for (const registeredListener of this.globalDocumentEventListeners) { + const eventType = registeredListener.type; + const eventListener = registeredListener.listener; + document.removeEventListener(eventType, eventListener); + } + this.globalDocumentEventListeners = []; + FocusManager.getFocusManager = this.oldGetFocusManager; } }