Skip to content

fix: Make workspace-multiselect plugin workable with Blockly v12 #9261

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 1 commit into
base: develop
Choose a base branch
from
+60 −32
Draft

fix: Make workspace-multiselect plugin workable with Blockly v12 #9261

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions core/block_svg.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,12 @@ export class BlockSvg

const oldXY = this.getRelativeToSurfaceXY();
const focusedNode = getFocusManager().getFocusedNode();
const restoreFocus = this.getSvgRoot().contains(
focusedNode?.getFocusableElement() ?? null,
);
let element = focusedNode?.getFocusableElement() ?? null;
// Make sure the element is of Node type
if (!(element instanceof Node)) {
element = null;
}
Comment on lines +304 to +307
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT this check does nothing (and I'm surprised that TSC does not complain), because IFocusableNode's getFocusableElement method is typed as returning HTMLElement | SVGElement and both HTMLElement and SVGElement inherit from Node, so the only time the body of the if statement will be executed is if element is already null.

If some focusable node's .getFocusableElement() returns something which is not an instanceof Node then it has violated the interface definition—unless I have greatly misunderstood something about how the DOM works, at any rate.

Copy link
Contributor Author

@HollowMan6 HollowMan6 Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If some focusable node's .getFocusableElement() returns something which is not an instanceof Node then it has violated the interface definition

For now the MultiselectDraggable is dummy and doesn't have an actual corresponding DOM object, that's why this check is added here, not sure if there's a better way for handling this one.

mit-cml/workspace-multiselect@1c9b6d5#diff-a6ef3f5605a5577d348743aa99d160ec6523f07f18c754f34e638513878515a5R47-R49

const restoreFocus = this.getSvgRoot().contains(element);
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible that the existing check, reproduced here, is overly-broad: I note that restoreFocus will get set to true even if this merely contains focusedNode (rather than is focusedNode).

I also wonder whether the focus manipulation in appendChild, which this section of code is working around, could itself be either eliminated or made more adept, such that this workaround would not be needed at all.

Copy link
Contributor Author

@HollowMan6 HollowMan6 Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For now, the MultiselectDraggable is dummy and doesn't have an actual corresponding DOM object. If this is properly addressed, I would assume this change will already be eliminated.

if (newParent) {
(newParent as BlockSvg).getSvgRoot().appendChild(svgRoot);
// appendChild() clears focus state, so re-focus the previously focused
Expand Down Expand Up @@ -870,11 +873,12 @@ export class BlockSvg
// If this block (or a descendant) was focused, focus its parent or
// workspace instead.
const focusManager = getFocusManager();
if (
this.getSvgRoot().contains(
focusManager.getFocusedNode()?.getFocusableElement() ?? null,
)
) {
let element = focusManager.getFocusedNode()?.getFocusableElement() ?? null;
// Make sure the element is of Node type
if (!(element instanceof Node)) {
element = null;
}
Comment on lines +877 to +880
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

if (this.getSvgRoot().contains(element)) {
let parent: BlockSvg | undefined | null = this.getParent();
if (!parent) {
// In some cases, blocks are disconnected from their parents before
Expand Down
19 changes: 12 additions & 7 deletions core/focus_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,13 +320,18 @@ export class FocusManager {
this.isUpdatingFocusedNode = true;
}

// Double check that state wasn't desynchronized in the background. See:
// https://github.com/google/blockly-keyboard-experimentation/issues/87.
// This is only done for the case where the same node is being focused twice
// since other cases should automatically correct (due to the rest of the
// routine running as normal).
const prevFocusedElement = this.focusedNode?.getFocusableElement();
const hasDesyncedState = prevFocusedElement !== document.activeElement;
// // TODO: This will break multiselect plugin as it will make the
// // selected `MultiselectDraggable` (has already assigned to `this.focusedNode`)
// // to lose focus (get changed to the actual focused one managed by Blockly core).
// // Double check that state wasn't desynchronized in the background. See:
// // https://github.com/google/blockly-keyboard-experimentation/issues/87.
// // This is only done for the case where the same node is being focused twice
// // since other cases should automatically correct (due to the rest of the
// // routine running as normal).
// const prevFocusedElement = this.focusedNode?.getFocusableElement();
// const hasDesyncedState = prevFocusedElement !== document.activeElement;
const hasDesyncedState = false;
Comment on lines +331 to +333
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The multiselect plugin will definitely need to be able to operate without causing focus desynchronisation.

Copy link
Contributor Author

@HollowMan6 HollowMan6 Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As explained:

Currently, the workspace-multiselect plugin acts as an adapter. It maintains its own multiple selection set (state), which keeps track of currently selected blocks. At the Blockly side, this is implemented as if there's a global MultiselectDraggable (which implements IDraggable) for each workspace, and current design for the plugin is that this instance of MultiselectDraggable should always be selected (in a dummy way as it won't create any new DOM object) when we have multiple blocks selected, so that the plugin can receive corresponding actions and pass all the actions to the blocks in the multiple selection set (state). It was working well in previous versions, but now in v12, the current code for getFocusManager().focusNode seems to break this completely, as it keeps unselecting MultiselectDraggable.

This change is an attempt to address the MultiselectDraggable unselecting issue, as we don't want the actual focus to get synchronised when in multiselect mode, although I'm aware that this will cause issues with other Blockly features.


if (this.focusedNode === focusableNode && !hasDesyncedState) {
if (mustRestoreUpdatingNode) {
// Reenable state syncing from DOM events.
Expand Down
27 changes: 16 additions & 11 deletions core/layer_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {getFocusManager} from './focus_manager.js';
import type {IFocusableNode} from './interfaces/i_focusable_node.js';
import {IRenderedElement} from './interfaces/i_rendered_element.js';
import * as layerNums from './layers.js';
Expand Down Expand Up @@ -104,11 +103,14 @@ export class LayerManager {
moveToDragLayer(elem: IRenderedElement & IFocusableNode) {
this.dragLayer?.appendChild(elem.getSvgRoot());

if (elem.canBeFocused()) {
// Since moving the element to the drag layer will cause it to lose focus,
// ensure it regains focus (to ensure proper highlights & sent events).
getFocusManager().focusNode(elem);
}
// // TODO: This will break multiselect plugin, as multiselect plugin will
// // implicitly call this method as well, we should not change the focus to individual
// // block and move away from `MultiselectDraggable` at this time.
// if (elem.canBeFocused()) {
// // Since moving the element to the drag layer will cause it to lose focus,
// // ensure it regains focus (to ensure proper highlights & sent events).
// getFocusManager().focusNode(elem);
Comment on lines +110 to +112
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not obvious to me why moving an element to the drag layer will cause it to lose focus. Maybe that is the problem that should be fixed, so this section of code is not needed.

Copy link
Contributor Author

@HollowMan6 HollowMan6 Jul 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because that we should keep MultiselectDraggable selected when we have multiple blocks selected, and when we do the dragging, we drag multiple blocks one by one by calling the method here. This code will make the single exact block that is in the dragging process get selected, instead of keeping MultiselectDraggable selected.

// }
}

/**
Expand All @@ -119,11 +121,14 @@ export class LayerManager {
moveOffDragLayer(elem: IRenderedElement & IFocusableNode, layerNum: number) {
this.append(elem, layerNum);

if (elem.canBeFocused()) {
// Since moving the element off the drag layer will cause it to lose focus,
// ensure it regains focus (to ensure proper highlights & sent events).
getFocusManager().focusNode(elem);
}
// // TODO: This will break multiselect plugin, as multiselect plugin will
// // implicitly call this method as well, we should not change the focus to individual
// // block and move away from `MultiselectDraggable` at this time.
// if (elem.canBeFocused()) {
// // Since moving the element off the drag layer will cause it to lose focus,
// // ensure it regains focus (to ensure proper highlights & sent events).
// getFocusManager().focusNode(elem);
// }
Comment on lines +128 to +131
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

}

/**
Expand Down
26 changes: 20 additions & 6 deletions core/utils/dom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,16 @@ export function createSvgElement<T extends SVGElement>(
*/
export function addClass(element: Element, className: string): boolean {
const classNames = className.split(' ');
if (classNames.every((name) => element.classList.contains(name))) {
if (
classNames.every(
(name) => !!element.classList && element.classList.contains(name),
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MDN tells me that Element's .classList will always be a DOMTokenList, even if the element has no class attribute, so I cannot see why the nullish check might be needed here.

Copy link
Contributor Author

@HollowMan6 HollowMan6 Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar reason as for now, the MultiselectDraggable is dummy and doesn't have an actual corresponding DOM object.

)
) {
return false;
}
element.classList.add(...classNames);
if (element.classList) {
element.classList.add(...classNames);
}
return true;
}

Expand All @@ -90,7 +96,9 @@ export function addClass(element: Element, className: string): boolean {
* @param classNames A string of one or multiple class names for an element.
*/
export function removeClasses(element: Element, classNames: string) {
element.classList.remove(...classNames.split(' '));
if (element.classList) {
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

element.classList.remove(...classNames.split(' '));
}
}

/**
Expand All @@ -104,10 +112,16 @@ export function removeClasses(element: Element, classNames: string) {
*/
export function removeClass(element: Element, className: string): boolean {
const classNames = className.split(' ');
if (classNames.every((name) => !element.classList.contains(name))) {
if (
classNames.every(
(name) => !element.classList || !element.classList.contains(name),
Copy link
Contributor

@cpcallen cpcallen Jul 29, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto here.

)
) {
return false;
}
element.classList.remove(...classNames);
if (element.classList) {
element.classList.remove(...classNames);
}
return true;
}

Expand All @@ -119,7 +133,7 @@ export function removeClass(element: Element, className: string): boolean {
* @returns True if class exists, false otherwise.
*/
export function hasClass(element: Element, className: string): boolean {
return element.classList.contains(className);
return !!element.classList && element.classList.contains(className);
}

/**
Expand Down