fix: Make workspace-multiselect plugin workable with Blockly v12 #9261
Conversation
Signed-off-by: Hollow Man <[email protected]>
| // Make sure the element is of Node type | ||
| if (!(element instanceof Node)) { | ||
| element = null; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // Make sure the element is of Node type | ||
| if (!(element instanceof Node)) { | ||
| element = null; | ||
| } |
| // const prevFocusedElement = this.focusedNode?.getFocusableElement(); | ||
| // const hasDesyncedState = prevFocusedElement !== document.activeElement; | ||
| const hasDesyncedState = false; |
There was a problem hiding this comment.
The multiselect plugin will definitely need to be able to operate without causing focus desynchronisation.
There was a problem hiding this comment.
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 ofMultiselectDraggableshould 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 forgetFocusManager().focusNodeseems to break this completely, as it keeps unselectingMultiselectDraggable.
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 (!(element instanceof Node)) { | ||
| element = null; | ||
| } | ||
| const restoreFocus = this.getSvgRoot().contains(element); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // // 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| // // 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); | ||
| // } |
| if (classNames.every((name) => element.classList.contains(name))) { | ||
| if ( | ||
| classNames.every( | ||
| (name) => !!element.classList && element.classList.contains(name), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Similar reason as for now, the MultiselectDraggable is dummy and doesn't have an actual corresponding DOM object.
| @@ -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) { | |||
| if (classNames.every((name) => !element.classList.contains(name))) { | ||
| if ( | ||
| classNames.every( | ||
| (name) => !element.classList || !element.classList.contains(name), |
The basics
The details
Resolves
Fixes mit-cml/workspace-multiselect#103
Proposed Changes
Workspace-multiselect plugin is in the process of updating to Blockly v12:
However, some changes are necessary to get Blockly working properly with the plugin. The fundamental issue here is that Blockly v12 is buggy at this stage to properly support a custom and dummy IDraggable (
MultiselectDraggablefor the plugin's case), as Blockly constantly wants to change the focus to the actual block via hardcodedgetFocusManager().focusNodecalls that are scattered all around the codebase. SettingMultiselectDraggabledirectly viaBlockly.common.setSelecteddoesn't work as well, and we will need to callBlockly.getFocusManager().updateFocusedNodedirectly.I'm sure the changes in this PR now will break some native Blockly features here and there. Feel free to modify on top of this PR directly, and I'll help test out to see if it still works with the plugin.
Reason for Changes
Necessary changes to make workspace-multiselect plugin workable with Blockly v12
Test Coverage
N/A
Documentation
N/A
Additional Information
https://groups.google.com/g/blockly/c/7RSn9cXAulA