From 870db460520456feaf0d694517bee4219574ff85 Mon Sep 17 00:00:00 2001 From: Grace Date: Fri, 21 Mar 2025 10:37:26 +0000 Subject: [PATCH 1/5] fix: right click menu issues PR #326 introduced an issue with stale current node when showing the context menu by mouse. It's only possible to fix this by reinstating the updating of curNode from the selection. We also attempt to avoid showing passive focus when using the context menu and widgets (via mouse and keyboard) for consistency with the normal Blockly experience. Fixes an issue clicking on shadow blocks which could previously show an outline around the shadow and parent block. --- src/actions/delete.ts | 2 +- src/index.ts | 28 ++++++++++++++++ src/line_cursor.ts | 62 ++++++++++++++++++++++++++---------- src/navigation.ts | 42 ++++++++++++++++++++++-- src/navigation_controller.ts | 7 ++++ 5 files changed, 121 insertions(+), 20 deletions(-) diff --git a/src/actions/delete.ts b/src/actions/delete.ts index 7b6f2a9d..245844f6 100644 --- a/src/actions/delete.ts +++ b/src/actions/delete.ts @@ -150,7 +150,7 @@ export class DeleteAction { private deletePrecondition(workspace: WorkspaceSvg) { if (!this.canCurrentlyEdit(workspace)) return false; - const sourceBlock = workspace.getCursor()?.getCurNode().getSourceBlock(); + const sourceBlock = workspace.getCursor()?.getCurNode()?.getSourceBlock(); return !!sourceBlock?.isDeletable(); } diff --git a/src/index.ts b/src/index.ts index b0060689..7decd1fb 100644 --- a/src/index.ts +++ b/src/index.ts @@ -30,6 +30,9 @@ export class KeyboardNavigation { /** Event handler run when the workspace loses focus. */ private blurListener: (e: Event) => void; + /** Event handler run when the widget or dropdown div loses focus. */ + private widgetDropDownDivFocusOutListener: (e: Event) => void; + /** Event handler run when the toolbox gains focus. */ private toolboxFocusListener: () => void; @@ -142,6 +145,22 @@ export class KeyboardNavigation { workspace.getSvgGroup().addEventListener('focus', this.focusListener); workspace.getSvgGroup().addEventListener('blur', this.blurListener); + this.widgetDropDownDivFocusOutListener = (e: Event) => { + this.navigationController.handleFocusOutWidgetDropdownDiv( + workspace, + (e as FocusEvent).relatedTarget, + ); + }; + + Blockly.WidgetDiv.getDiv()?.addEventListener( + 'focusout', + this.widgetDropDownDivFocusOutListener, + ); + Blockly.DropDownDiv.getContentDiv()?.addEventListener( + 'focusout', + this.widgetDropDownDivFocusOutListener, + ); + const toolboxElement = getToolboxElement(workspace); this.toolboxFocusListener = () => { this.navigationController.handleFocusToolbox(workspace); @@ -197,6 +216,15 @@ export class KeyboardNavigation { .getSvgGroup() .removeEventListener('focus', this.focusListener); + Blockly.WidgetDiv.getDiv()?.removeEventListener( + 'focusout', + this.widgetDropDownDivFocusOutListener, + ); + Blockly.DropDownDiv.getContentDiv()?.removeEventListener( + 'focusout', + this.widgetDropDownDivFocusOutListener, + ); + const toolboxElement = getToolboxElement(this.workspace); toolboxElement?.removeEventListener('focus', this.toolboxFocusListener); toolboxElement?.removeEventListener('blur', this.toolboxBlurListener); diff --git a/src/line_cursor.ts b/src/line_cursor.ts index 4eebf195..8781c1ad 100644 --- a/src/line_cursor.ts +++ b/src/line_cursor.ts @@ -474,6 +474,43 @@ export class LineCursor extends Marker { throw new Error('no valid nodes in this.potentialNodes'); } + /** + * Get the current location of the cursor. + * + * Overrides normal Marker getCurNode to update the current node from the selected + * block. We typically update the node from the selection via a listener but + * that is not called immediately when `Gesture` calls `Blockly.common.setSelected`. + * In particular the listener runs after showing the context menu. + * + * @returns The current field, connection, or block the cursor is on. + */ + override getCurNode(): ASTNode { + const curNode = super.getCurNode(); + let selected = Blockly.common.getSelected(); + if ( + selected === null && + curNode?.getType() === Blockly.ASTNode.types.BLOCK + ) { + // Selection says our curNode is not selected anymore. + // this.setCurNode(null as never, true); + return super.getCurNode(); + } + if (selected?.workspace !== this.workspace) return curNode; + // Selection has a block in our workspace. + if (selected instanceof Blockly.BlockSvg) { + if (selected.isShadow()) { + // Although the shadow block is selected it's the parent that has the + // visual selection. + selected = selected.getParent(); + } + if (selected) { + this.setCurNode(new ASTNode(ASTNode.types.BLOCK, selected), true); + } + } + + return super.getCurNode(); + } + /** * Set the location of the cursor and draw it. * @@ -481,14 +518,13 @@ export class LineCursor extends Marker { * this.drawMarker() instead of this.drawer.draw() directly. * * @param newNode The new location of the cursor. + * @param selectionUpToDate If false (the default) we'll update the selection too. */ - override setCurNode(newNode: ASTNode, selectionInSync = false) { - if (newNode?.getLocation() === this.getCurNode()?.getLocation()) { - return; - } - if (!selectionInSync) { + override setCurNode(newNode: ASTNode, selectionUpToDate = false) { + if (!selectionUpToDate) { if ( newNode?.getType() === ASTNode.types.BLOCK && + // For shadow blocks we need to clear the selection that's drawn on their parent. !(newNode.getLocation() as Blockly.BlockSvg).isShadow() ) { if (Blockly.common.getSelected() !== newNode.getLocation()) { @@ -659,22 +695,16 @@ export class LineCursor extends Marker { /** * Event listener that syncs the cursor location to the selected * block on SELECTED events. + * + * @param event The `Selected` event. */ private selectListener(event: Blockly.Events.Abstract) { if (event.type !== Blockly.Events.SELECTED) return; const selectedEvent = event as Blockly.Events.Selected; if (selectedEvent.workspaceId !== this.workspace.id) return; - if (selectedEvent.newElementId) { - const block = this.workspace.getBlockById(selectedEvent.newElementId); - if (block) { - const node = ASTNode.createBlockNode(block); - if (node) { - this.setCurNode(node, true); - } - } - } else { - this.setCurNode(null as never, true); - } + // This runs too late so the logic to update the selection is in + // `getCurNode`. + this.getCurNode(); } } diff --git a/src/navigation.ts b/src/navigation.ts index 5d97409a..348c234b 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -415,14 +415,22 @@ export class Navigation { } /** - * Clears the navigation state and switches to using the passive focus indicator. + * Clears navigation state and switches to using the passive focus indicator + * if it is not the context menu / field input that is causing blur. * * @param workspace The workspace that has lost focus. + * @param ignorePopUpDivs Whether to skip the focus indicator change when + * the widget/dropdown divs are open. */ - handleBlurWorkspace(workspace: Blockly.WorkspaceSvg) { + handleBlurWorkspace( + workspace: Blockly.WorkspaceSvg, + ignorePopUpDivs = false, + ) { this.setState(workspace, Constants.STATE.NOWHERE); const cursor = workspace.getCursor(); - if (cursor) { + const popUpDivsShowing = + Blockly.WidgetDiv.isVisible() || Blockly.DropDownDiv.isVisible(); + if (cursor && (ignorePopUpDivs || !popUpDivsShowing)) { if (cursor.getCurNode()) { this.passiveFocusIndicator.show(cursor.getCurNode()); } @@ -431,6 +439,34 @@ export class Navigation { } } + /** + * Handle the widget or dropdown div losing focus (via focusout). + * + * Because we skip the widget/dropdown div cases in `handleBlurWorkspace` we need + * to catch them here. + * + * @param workspace The workspace. + * @param relatedTarget The related target (newly focused element if any). + */ + handleFocusOutWidgetDropdownDiv( + workspace: Blockly.WorkspaceSvg, + relatedTarget: EventTarget | null, + ) { + if (relatedTarget === null) { + // Workaround: + // Skip document.body/null case until these blur bugs are fixed to avoid + // flipping to passive focus as the user moves their mouse over the + // dropdown/widget at the cost of clicks on body showing the wrong focus + // style. + // https://github.com/google/blockly-samples/issues/2498 + // https://github.com/google/blockly/issues/8819 + return; + } + if (relatedTarget !== getWorkspaceElement(workspace)) { + this.handleBlurWorkspace(workspace, true); + } + } + /** * Sets browser focus to the toolbox (if any). * diff --git a/src/navigation_controller.ts b/src/navigation_controller.ts index b5c6c9ab..9d9d2c62 100644 --- a/src/navigation_controller.ts +++ b/src/navigation_controller.ts @@ -196,6 +196,13 @@ export class NavigationController { this.navigation.handleBlurWorkspace(workspace); } + handleFocusOutWidgetDropdownDiv( + workspace: Blockly.WorkspaceSvg, + relatedTarget: EventTarget | null, + ) { + this.navigation.handleFocusOutWidgetDropdownDiv(workspace, relatedTarget); + } + focusToolbox(workspace: Blockly.WorkspaceSvg) { this.navigation.focusToolbox(workspace); } From 1870e5e536f6124012a3a33278173e3317d32cbc Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Tue, 25 Mar 2025 15:34:23 +0000 Subject: [PATCH 2/5] chore: remove reference to fixed bug --- src/navigation.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/navigation.ts b/src/navigation.ts index da323d23..91750e6d 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -455,12 +455,10 @@ export class Navigation { ) { if (relatedTarget === null) { // Workaround: - // Skip document.body/null case until these blur bugs are fixed to avoid + // Skip document.body/null case until this blur bugs is fixed to avoid // flipping to passive focus as the user moves their mouse over the - // dropdown/widget at the cost of clicks on body showing the wrong focus - // style. + // colour picker. // https://github.com/google/blockly-samples/issues/2498 - // https://github.com/google/blockly/issues/8819 return; } if (relatedTarget !== getWorkspaceElement(workspace)) { From 586a6f0734847fc76e0871619aba6a615f8ae17d Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 27 Mar 2025 11:30:10 +0000 Subject: [PATCH 3/5] chore: typo in comment --- src/navigation.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/navigation.ts b/src/navigation.ts index 91750e6d..bb02f1a1 100644 --- a/src/navigation.ts +++ b/src/navigation.ts @@ -455,7 +455,7 @@ export class Navigation { ) { if (relatedTarget === null) { // Workaround: - // Skip document.body/null case until this blur bugs is fixed to avoid + // Skip document.body/null case until this blur bug is fixed to avoid // flipping to passive focus as the user moves their mouse over the // colour picker. // https://github.com/google/blockly-samples/issues/2498 From c2e7d4b161bd5e5269b62df93d78c42a27d73a58 Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 27 Mar 2025 11:28:17 +0000 Subject: [PATCH 4/5] fix: null node case for action menu --- src/actions/action_menu.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/actions/action_menu.ts b/src/actions/action_menu.ts index 5eab1325..584624d7 100644 --- a/src/actions/action_menu.ts +++ b/src/actions/action_menu.ts @@ -111,7 +111,7 @@ export class ActionMenu { const cursor = workspace.getCursor(); if (!cursor) throw new Error('workspace has no cursor'); const node = cursor.getCurNode(); - if (!node) throw new Error('No node is currently selected'); + if (!node) return false; const nodeType = node.getType(); switch (nodeType) { case ASTNode.types.BLOCK: From 1233f578fd94a0941a8e028a0417d7e83a3eda3f Mon Sep 17 00:00:00 2001 From: Matt Hillsdon Date: Thu, 27 Mar 2025 10:56:23 +0000 Subject: [PATCH 5/5] fix: stale curNode when clicking the workspace Due to an unintentially commented out line, clicking the workspace wouldn't clear the current node so e.g. delete would act on a block that didn't have a visual selection. Re-enabling that required revisiting shadow blocks to prevent getCurNode unexpectedly clearing based on a null selection. We now always set/use the selection for shadow blocks and take care not to move the cursor unexpectedly as it can be in two positions for a shadow block selection. --- src/line_cursor.ts | 158 +++++++++++++++++++++++++++++---------------- 1 file changed, 101 insertions(+), 57 deletions(-) diff --git a/src/line_cursor.ts b/src/line_cursor.ts index 37055c33..54508590 100644 --- a/src/line_cursor.ts +++ b/src/line_cursor.ts @@ -15,7 +15,7 @@ import * as Blockly from 'blockly/core'; import {ASTNode, Marker} from 'blockly/core'; -import {getWorkspaceElement, scrollBoundsIntoView} from './workspace_utilities'; +import {scrollBoundsIntoView} from './workspace_utilities'; /** Options object for LineCursor instances. */ export type CursorOptions = { @@ -61,7 +61,7 @@ export class LineCursor extends Marker { ) { super(); // Bind selectListener to facilitate future install/uninstall. - this.selectListener = this.selectListener.bind(this); + this.changeListener = this.changeListener.bind(this); // Regularise options and apply defaults. this.options = {...defaultOptions, ...options}; @@ -81,7 +81,7 @@ export class LineCursor extends Marker { markerManager.setCursor(this); const oldCursorNode = this.oldCursor?.getCurNode(); if (oldCursorNode) this.setCurNode(oldCursorNode); - this.workspace.addChangeListener(this.selectListener); + this.workspace.addChangeListener(this.changeListener); this.installed = true; } @@ -92,7 +92,7 @@ export class LineCursor extends Marker { */ uninstall() { if (!this.installed) throw new Error('LineCursor not yet installed'); - this.workspace.removeChangeListener(this.selectListener.bind(this)); + this.workspace.removeChangeListener(this.changeListener.bind(this)); if (this.oldCursor) { this.workspace.getMarkerManager().setCursor(this.oldCursor); } @@ -479,36 +479,14 @@ export class LineCursor extends Marker { * Get the current location of the cursor. * * Overrides normal Marker getCurNode to update the current node from the selected - * block. We typically update the node from the selection via a listener but - * that is not called immediately when `Gesture` calls `Blockly.common.setSelected`. + * block. This typically happens via the selection listener but that is not called + * immediately when `Gesture` calls `Blockly.common.setSelected`. * In particular the listener runs after showing the context menu. * * @returns The current field, connection, or block the cursor is on. */ - override getCurNode(): ASTNode | null { - const curNode = super.getCurNode(); - let selected = Blockly.common.getSelected(); - if ( - selected === null && - curNode?.getType() === Blockly.ASTNode.types.BLOCK - ) { - // Selection says our curNode is not selected anymore. - // this.setCurNode(null as never, true); - return super.getCurNode(); - } - if (selected?.workspace !== this.workspace) return curNode; - // Selection has a block in our workspace. - if (selected instanceof Blockly.BlockSvg) { - if (selected.isShadow()) { - // Although the shadow block is selected it's the parent that has the - // visual selection. - selected = selected.getParent(); - } - if (selected) { - this.setCurNode(new ASTNode(ASTNode.types.BLOCK, selected), true); - } - } - + override getCurNode(): Blockly.ASTNode | null { + this.updateCurNodeFromSelection(); return super.getCurNode(); } @@ -555,25 +533,9 @@ export class LineCursor extends Marker { * @param newNode The new location of the cursor. * @param selectionUpToDate If false (the default) we'll update the selection too. */ - override setCurNode(newNode: ASTNode, selectionUpToDate = false) { + override setCurNode(newNode: ASTNode | null, selectionUpToDate = false) { if (!selectionUpToDate) { - if ( - newNode?.getType() === ASTNode.types.BLOCK && - // For shadow blocks we need to clear the selection that's drawn on their parent. - !(newNode.getLocation() as Blockly.BlockSvg).isShadow() - ) { - if (Blockly.common.getSelected() !== newNode.getLocation()) { - Blockly.Events.disable(); - Blockly.common.setSelected(newNode.getLocation() as Blockly.BlockSvg); - Blockly.Events.enable(); - } - } else { - if (Blockly.common.getSelected()) { - Blockly.Events.disable(); - Blockly.common.setSelected(null); - Blockly.Events.enable(); - } - } + this.updateSelectionFromNode(newNode); } super.setCurNode(newNode); @@ -652,6 +614,7 @@ export class LineCursor extends Marker { // Selection should already be in sync. } else { block.addSelect(); + block.getParent()?.removeSelect(); } } @@ -707,18 +670,99 @@ export class LineCursor extends Marker { } /** - * Event listener that syncs the cursor location to the selected - * block on SELECTED events. + * 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 selectListener(event: Blockly.Events.Abstract) { - if (event.type !== Blockly.Events.SELECTED) return; - const selectedEvent = event as Blockly.Events.Selected; - if (selectedEvent.workspaceId !== this.workspace.id) return; - // This runs too late so the logic to update the selection is in - // `getCurNode`. - this.getCurNode(); + private changeListener(event: Blockly.Events.Abstract) { + switch (event.type) { + case Blockly.Events.SELECTED: + this.updateCurNodeFromSelection(); + break; + case Blockly.Events.CLICK: { + const click = event as Blockly.Events.Click; + if ( + click.workspaceId === this.workspace.id && + click.targetType === Blockly.Events.ClickTarget.WORKSPACE + ) { + this.setCurNode(null); + } + } + } + } + + /** + * Updates the current node to match the selection. + * + * Clears the current node if it's on a block but the selection is null. + * Sets the node to a block if selected for our workspace. + * For shadow blocks selections the parent is used by default (unless we're + * already on the shadow block via keyboard) as that's where the visual + * selection is. + */ + private updateCurNodeFromSelection() { + const curNode = super.getCurNode(); + const selected = Blockly.common.getSelected(); + + if ( + selected === null && + curNode?.getType() === Blockly.ASTNode.types.BLOCK + ) { + this.setCurNode(null, true); + return; + } + if (selected?.workspace !== this.workspace) { + return; + } + if (selected instanceof Blockly.BlockSvg) { + let block: Blockly.BlockSvg | null = selected; + if (selected.isShadow()) { + // OK if the current node is on the parent OR the shadow block. + // The former happens for clicks, the latter for keyboard nav. + if ( + curNode && + (curNode.getLocation() === block || + curNode.getLocation() === block.getParent()) + ) { + return; + } + block = block.getParent(); + } + if (block) { + this.setCurNode(Blockly.ASTNode.createBlockNode(block)!, true); + } + } + } + + /** + * Updates the selection from the node. + * + * 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. + */ + private updateSelectionFromNode(newNode: Blockly.ASTNode | null) { + if (newNode?.getType() === ASTNode.types.BLOCK) { + if (Blockly.common.getSelected() !== newNode.getLocation()) { + Blockly.Events.disable(); + Blockly.common.setSelected(newNode.getLocation() as Blockly.BlockSvg); + Blockly.Events.enable(); + } + } else { + if (Blockly.common.getSelected()) { + Blockly.Events.disable(); + Blockly.common.setSelected(null); + Blockly.Events.enable(); + } + } } }