Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
4 changes: 2 additions & 2 deletions blocks/procedures.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import type {Block} from '../core/block.js';
import type {BlockSvg} from '../core/block_svg.js';
import type {BlockDefinition} from '../core/blocks.js';
import * as common from '../core/common.js';
import {defineBlocks} from '../core/common.js';
import {config} from '../core/config.js';
import type {Connection} from '../core/connection.js';
Expand All @@ -27,6 +26,7 @@ import {FieldCheckbox} from '../core/field_checkbox.js';
import {FieldLabel} from '../core/field_label.js';
import * as fieldRegistry from '../core/field_registry.js';
import {FieldTextInput} from '../core/field_textinput.js';
import {getFocusManager} from '../core/focus_manager.js';
import '../core/icons/comment_icon.js';
import {MutatorIcon as Mutator} from '../core/icons/mutator_icon.js';
import '../core/icons/warning_icon.js';
Expand Down Expand Up @@ -1178,7 +1178,7 @@ const PROCEDURE_CALL_COMMON = {
const def = Procedures.getDefinition(name, workspace);
if (def) {
(workspace as WorkspaceSvg).centerOnBlock(def.id);
common.setSelected(def as BlockSvg);
getFocusManager().focusNode(def as BlockSvg);
}
},
});
Expand Down
38 changes: 5 additions & 33 deletions core/block_svg.ts
Original file line number Diff line number Diff line change
Expand Up @@ -265,20 +265,14 @@ export class BlockSvg

/** Selects this block. Highlights the block visually. */
select() {
if (this.isShadow()) {
Comment thread
BenHenning marked this conversation as resolved.
this.getParent()?.select();
return;
}
this.addSelect();
common.fireSelectedEvent(this);
}

/** Unselects this block. Unhighlights the block visually. */
unselect() {
if (this.isShadow()) {
this.getParent()?.unselect();
return;
}
this.removeSelect();
common.fireSelectedEvent(null);
}

/**
Expand Down Expand Up @@ -861,25 +855,6 @@ export class BlockSvg
blockAnimations.disposeUiEffect(this);
}

// Selecting a shadow block highlights an ancestor block, but that highlight
// should be removed if the shadow block will be deleted. So, before
// deleting blocks and severing the connections between them, check whether
// doing so would delete a selected block and make sure that any associated
// parent is updated.
const selection = common.getSelected();
if (selection instanceof Block) {
let selectionAncestor: Block | null = selection;
while (selectionAncestor !== null) {
if (selectionAncestor === this) {
// The block to be deleted contains the selected block, so remove any
// selection highlight associated with the selected block before
// deleting them.
selection.unselect();
}
selectionAncestor = selectionAncestor.getParent();
}
}

super.dispose(!!healStack);
dom.removeNode(this.svgGroup);
}
Expand All @@ -892,8 +867,7 @@ export class BlockSvg
this.disposing = true;
super.disposeInternal();

if (common.getSelected() === this) {
this.unselect();
if (getFocusManager().getFocusedNode() === this) {
Comment thread
maribethb marked this conversation as resolved.
this.workspace.cancelCurrentGesture();
}

Expand Down Expand Up @@ -1877,13 +1851,11 @@ export class BlockSvg

/** See IFocusableNode.onNodeFocus. */
onNodeFocus(): void {
common.setSelected(this);
this.select();
}

/** See IFocusableNode.onNodeBlur. */
onNodeBlur(): void {
if (common.getSelected() === this) {
common.setSelected(null);
}
this.unselect();
}
}
28 changes: 25 additions & 3 deletions core/bubbles/bubble.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* SPDX-License-Identifier: Apache-2.0
*/

import {ISelectable} from '../blockly.js';
import {IFocusableTree, ISelectable, getFocusManager} from '../blockly.js';
import * as browserEvents from '../browser_events.js';
import * as common from '../common.js';
import {BubbleDragStrategy} from '../dragging/bubble_drag_strategy.js';
Expand Down Expand Up @@ -101,7 +101,7 @@ export abstract class Bubble implements IBubble, ISelectable {
this.id = idGenerator.getNextUniqueId();
this.svgRoot = dom.createSvgElement(
Svg.G,
{'class': 'blocklyBubble'},
{'class': 'blocklyBubble', 'tabindex': '-1', 'id': this.id},
workspace.getBubbleCanvas(),
);
const embossGroup = dom.createSvgElement(
Expand Down Expand Up @@ -212,7 +212,7 @@ export abstract class Bubble implements IBubble, ISelectable {
private onMouseDown(e: PointerEvent) {
this.workspace.getGesture(e)?.handleBubbleStart(e, this);
this.bringToFront();
common.setSelected(this);
getFocusManager().focusNode(this);
}

/** Positions the bubble relative to its anchor. Does not render its tail. */
Expand Down Expand Up @@ -647,9 +647,31 @@ export abstract class Bubble implements IBubble, ISelectable {

select(): void {
// Bubbles don't have any visual for being selected.
common.fireSelectedEvent(this);
}

unselect(): void {
// Bubbles don't have any visual for being selected.
common.fireSelectedEvent(null);
}

/** See IFocusableNode.getFocusableElement. */
getFocusableElement(): HTMLElement | SVGElement {
return this.svgRoot;
}

/** See IFocusableNode.getFocusableTree. */
getFocusableTree(): IFocusableTree {
return this.workspace;
}

/** See IFocusableNode.onNodeFocus. */
onNodeFocus(): void {
this.select();
}

/** See IFocusableNode.onNodeBlur. */
onNodeBlur(): void {
this.unselect();
}
}
4 changes: 2 additions & 2 deletions core/clipboard/workspace_comment_paster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
*/

import {RenderedWorkspaceComment} from '../comments/rendered_workspace_comment.js';
import * as common from '../common.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 * as commentSerialiation from '../serialization/workspace_comments.js';
Expand Down Expand Up @@ -49,7 +49,7 @@ export class WorkspaceCommentPaster
if (eventUtils.isEnabled()) {
eventUtils.fire(new (eventUtils.get(EventType.COMMENT_CREATE))(comment));
}
common.setSelected(comment);
getFocusManager().focusNode(comment);
Comment thread
BenHenning marked this conversation as resolved.
return comment;
}
}
Expand Down
28 changes: 27 additions & 1 deletion core/comments/rendered_workspace_comment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,13 @@ import * as common from '../common.js';
import * as contextMenu from '../contextmenu.js';
import {ContextMenuRegistry} from '../contextmenu_registry.js';
import {CommentDragStrategy} from '../dragging/comment_drag_strategy.js';
import {getFocusManager} from '../focus_manager.js';
import {IBoundedElement} from '../interfaces/i_bounded_element.js';
import {IContextMenu} from '../interfaces/i_contextmenu.js';
import {ICopyable} from '../interfaces/i_copyable.js';
import {IDeletable} from '../interfaces/i_deletable.js';
import {IDraggable} from '../interfaces/i_draggable.js';
import {IFocusableTree} from '../interfaces/i_focusable_tree.js';
import {IRenderedElement} from '../interfaces/i_rendered_element.js';
import {ISelectable} from '../interfaces/i_selectable.js';
import * as layers from '../layers.js';
Expand Down Expand Up @@ -60,6 +62,8 @@ export class RenderedWorkspaceComment
this.view.setSize(this.getSize());
this.view.setEditable(this.isEditable());
this.view.getSvgRoot().setAttribute('data-id', this.id);
this.view.getSvgRoot().setAttribute('id', this.id);
this.view.getSvgRoot().setAttribute('tabindex', '-1');

this.addModelUpdateBindings();

Expand Down Expand Up @@ -222,7 +226,7 @@ export class RenderedWorkspaceComment
gesture.handleCommentStart(e, this);
this.workspace.getLayerManager()?.append(this, layers.BLOCK);
}
common.setSelected(this);
getFocusManager().focusNode(this);
}
}

Expand Down Expand Up @@ -263,11 +267,13 @@ export class RenderedWorkspaceComment
/** Visually highlights the comment. */
select(): void {
dom.addClass(this.getSvgRoot(), 'blocklySelected');
common.fireSelectedEvent(this);
}

/** Visually unhighlights the comment. */
unselect(): void {
dom.removeClass(this.getSvgRoot(), 'blocklySelected');
common.fireSelectedEvent(null);
}

/**
Expand Down Expand Up @@ -322,4 +328,24 @@ export class RenderedWorkspaceComment
this.moveTo(alignedXY, ['snap']);
}
}

/** See IFocusableNode.getFocusableElement. */
getFocusableElement(): HTMLElement | SVGElement {
return this.getSvgRoot();
}

/** See IFocusableNode.getFocusableTree. */
getFocusableTree(): IFocusableTree {
return this.workspace;
}

/** See IFocusableNode.onNodeFocus. */
onNodeFocus(): void {
this.select();
}

/** See IFocusableNode.onNodeBlur. */
onNodeBlur(): void {
this.unselect();
}
}
57 changes: 41 additions & 16 deletions core/common.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,12 @@
// Former goog.module ID: Blockly.common

import type {Block} from './block.js';
import {ISelectable} from './blockly.js';
import {
BlockSvg,
ISelectable,
getFocusManager,
isSelectable,
} from './blockly.js';
Comment thread
BenHenning marked this conversation as resolved.
Outdated
import {BlockDefinition, Blocks} from './blocks.js';
import type {Connection} from './connection.js';
import {EventType} from './events/type.js';
Expand Down Expand Up @@ -86,38 +91,58 @@ export function setMainWorkspace(workspace: Workspace) {
}

/**
* Currently selected copyable object.
* Returns the current selection.
*/
let selected: ISelectable | null = null;
export function getSelected(): ISelectable | null {
const focused = getFocusManager().getFocusedNode();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am curious about how well this will work.

One scenario is that in context menu items sometimes the callback would use getSelected, because the block is still selected when the menu is open. But the block won’t be focused when the menu is open. So we would need to call this out as a breaking change. I am curious if there are other situations where a block would have been selected but not (active) focused and I’m not sure I can think of any, maybe in some widget div scenarios

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I think the scenario you describe would be broken as of #8938 not this change, but it's equally relevant. It's definitely the case now that right clicking will not maintain block selection since the selection highlight is cleared on focus loss.

It's something worth discussing, I think. Does something really have selection if it doesn't have input focus? My assumption is no which is why the change moved in this direction. If the answer should be yes, then we need to approach the auto-selection differently.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think logically it's fine, but it is a big behavior change that the block isn't selected anymore while you're interacting with non-Blockly parts of the UI (e.g. context menu or dev console) so we just need to be super clear about that in our communication.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

What's the best way for me to indicate that bit (per being super clear)?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Update the PR description of this PR (and/or any others you've made that are breaking) to include a ## Breaking Changes section that explains 1) who is broken by the change 2) what they need to do in response to the breaking change.

So for the issue about the block not being selected anymore, provide workarounds like using the (now passively-) focused block instead of selected block, using the scope for context menu items and shortcuts instead of selected, etc.

For this PR, you might need other details like what new methods need to be implemented if any for bubbles, etc.

Then remind me to call this out in the handwritten release notes we'll attach to the github release/forum announcement as well.

if (focused && isSelectable(focused)) return focused;
return null;
}

/**
* Returns the currently selected copyable object.
* @internal
*/
Comment thread
BenHenning marked this conversation as resolved.
Outdated
export function getSelected(): ISelectable | null {
return selected;
export function getSelectedBlock(): BlockSvg | null {
Comment thread
maribethb marked this conversation as resolved.
Outdated
const selected = getSelected();
if (!selected || !(selected instanceof BlockSvg)) return null;
let nonShadow: BlockSvg | null = selected;
while (nonShadow && nonShadow instanceof BlockSvg && nonShadow.isShadow()) {
nonShadow = nonShadow.getParent();
}
return nonShadow;
}

/**
* Sets the currently selected block. This function does not visually mark the
* block as selected or fire the required events. If you wish to
* programmatically select a block, use `BlockSvg#select`.
* Sets the current selection.
*
* To clear the current selection, select another ISelectable or focus a
* non-selectable (like the workspace root node).
*
* @param newSelection The newly selected block.
* @param newSelection The new selection to make.
* @internal
*/
export function setSelected(newSelection: ISelectable | null) {
if (selected === newSelection) return;
export function setSelected(newSelection: ISelectable) {
getFocusManager().focusNode(newSelection);
}

/**
* Fires a selection change event based on the new selection.
*
* This is only expected to be called by ISelectable implementations and should
* always be called before updating the current selection state. It does not
* change focus or selection state.
*
* @param newSelection The new selection.
* @internal
*/
export function fireSelectedEvent(newSelection: ISelectable | null) {
const selected = getSelected();
const event = new (eventUtils.get(EventType.SELECTED))(
selected?.id ?? null,
newSelection?.id ?? null,
newSelection?.workspace.id ?? selected?.workspace.id ?? '',
);
eventUtils.fire(event);

selected?.unselect();
selected = newSelection;
selected?.select();
}

/**
Expand Down
4 changes: 2 additions & 2 deletions core/contextmenu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
import type {Block} from './block.js';
import type {BlockSvg} from './block_svg.js';
import * as browserEvents from './browser_events.js';
import * as common from './common.js';
import {config} from './config.js';
import type {
ContextMenuOption,
LegacyContextMenuOption,
} from './contextmenu_registry.js';
import {EventType} from './events/type.js';
import * as eventUtils from './events/utils.js';
import {getFocusManager} from './focus_manager.js';
import {Menu} from './menu.js';
import {MenuSeparator} from './menu_separator.js';
import {MenuItem} from './menuitem.js';
Expand Down Expand Up @@ -289,7 +289,7 @@ export function callbackFactory(
if (eventUtils.isEnabled() && !newBlock.isShadow()) {
eventUtils.fire(new (eventUtils.get(EventType.BLOCK_CREATE))(newBlock));
}
common.setSelected(newBlock);
getFocusManager().focusNode(newBlock);
Comment thread
BenHenning marked this conversation as resolved.
return newBlock;
};
}
4 changes: 2 additions & 2 deletions core/contextmenu_items.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
import type {BlockSvg} from './block_svg.js';
import * as clipboard from './clipboard.js';
import {RenderedWorkspaceComment} from './comments/rendered_workspace_comment.js';
import * as common from './common.js';
import {MANUALLY_DISABLED} from './constants.js';
import {
ContextMenuRegistry,
Expand All @@ -19,6 +18,7 @@ import {
import * as dialog from './dialog.js';
import * as Events from './events/events.js';
import * as eventUtils from './events/utils.js';
import {getFocusManager} from './focus_manager.js';
import {CommentIcon} from './icons/comment_icon.js';
import {Msg} from './msg.js';
import {StatementInput} from './renderers/zelos/zelos.js';
Expand Down Expand Up @@ -631,7 +631,7 @@ export function registerCommentCreate() {
workspace,
),
);
common.setSelected(comment);
getFocusManager().focusNode(comment);
Comment thread
BenHenning marked this conversation as resolved.
eventUtils.setGroup(false);
},
scopeType: ContextMenuRegistry.ScopeType.WORKSPACE,
Expand Down
Loading