Skip to content

fix: Improve flyout behavior #9576

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

Merged
gonfunko merged 6 commits into from
Feb 6, 2026
Merged

fix: Improve flyout behavior #9576

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0dcdfd2
fix: Don't suppress and re-fire events when dragging blocks from the …
gonfunko Jan 20, 2026
f0a2478
fix: Fix bug that could cause blocks to get bumped when dragging from…
gonfunko Jan 20, 2026
79cb632
fix: Fix bug that made it more difficult to drag blocks from scrollab…
gonfunko Jan 20, 2026
c57b87e
chore: Run formatter
gonfunko Jan 20, 2026
c78f5b4
refactor: Clean up `createBlock`
gonfunko Feb 6, 2026
d4c861c
fix: Hide chaff at the end of block creation
gonfunko Feb 6, 2026
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
43 changes: 8 additions & 35 deletions packages/blockly/core/flyout_base.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@ import * as dom from './utils/dom.js';
import * as idGenerator from './utils/idgenerator.js';
import {Svg} from './utils/svg.js';
import * as toolbox from './utils/toolbox.js';
import * as Variables from './variables.js';
import {WorkspaceSvg} from './workspace_svg.js';

/**
Expand Down Expand Up @@ -813,44 +812,16 @@ export abstract class Flyout
* @internal
*/
createBlock(originalBlock: BlockSvg): BlockSvg {
let newBlock = null;
eventUtils.disable();
const variablesBeforeCreation = this.targetWorkspace
.getVariableMap()
.getAllVariables();
this.targetWorkspace.setResizesEnabled(false);
try {
newBlock = this.placeNewBlock(originalBlock);
return this.placeNewBlock(originalBlock);

@maribethb maribethb Feb 6, 2026

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.

If the block is not successfully created then now the flyout is closed, where previously the event utils would be enabled but then the method would crash (without closing flyout)

I think if a block isn't ultimately created for whatever reason (not sure how likely this method is to throw an error in reality) it would be better for the flyout not to close.

i don't think you need to use a try anymore since we don't have to clean up the event utils. if placeNewBlock errors then just let it error. so you can hideChaff before you return the new block without relying on the finally block.

@gonfunko gonfunko Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch, updated; this left this method as nothing but hideChaff() and calling through to placeNewBlock(), so I inlined placeNewBlock() here as it was private and had no other callsites.

} finally {
eventUtils.enable();
}

// Close the flyout.
this.targetWorkspace.hideChaff();

const newVariables = Variables.getAddedVariables(
this.targetWorkspace,
variablesBeforeCreation,
);
this.targetWorkspace.hideChaff();

if (eventUtils.isEnabled()) {
eventUtils.setGroup(true);
// Fire a VarCreate event for each (if any) new variable created.
for (let i = 0; i < newVariables.length; i++) {
const thisVariable = newVariables[i];
eventUtils.fire(
new (eventUtils.get(EventType.VAR_CREATE))(thisVariable),
);
// Close the flyout.
if (this.autoClose) {

@maribethb maribethb Feb 6, 2026

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.

Doesn't hiding the chaff on the target workspace already close the flyout?

@gonfunko gonfunko Feb 6, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Indeed it does, this was left over from before, but removed accordingly!

this.hide();
}

// Block events come after var events, in case they refer to newly created
// variables.
eventUtils.fire(new (eventUtils.get(EventType.BLOCK_CREATE))(newBlock));
}
if (this.autoClose) {
this.hide();
}
return newBlock;
}

/**
Expand Down Expand Up @@ -890,7 +861,9 @@ export abstract class Flyout
const json = this.serializeBlock(oldBlock);
// Normally this resizes leading to weird jumps. Save it for terminateDrag.
targetWorkspace.setResizesEnabled(false);
const block = blocks.append(json, targetWorkspace) as BlockSvg;
const block = blocks.appendInternal(json, targetWorkspace, {
recordUndo: true,
}) as BlockSvg;

this.positionNewBlock(oldBlock, block);

Expand Down
29 changes: 12 additions & 17 deletions packages/blockly/core/gesture.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,24 +273,19 @@ export class Gesture {
throw new Error(`Cannot update dragging from the flyout because the ' +
'flyout's target workspace is undefined`);
}
if (
!this.flyout.isScrollable() ||
this.flyout.isDragTowardWorkspace(this.currentDragDeltaXY)
) {
this.startWorkspace_ = this.flyout.targetWorkspace;
this.startWorkspace_.updateScreenCalculationsIfScrolled();
// Start the event group now, so that the same event group is used for
// block creation and block dragging.
if (!eventUtils.getGroup()) {
eventUtils.setGroup(true);
}
// The start block is no longer relevant, because this is a drag.
this.startBlock = null;
this.targetBlock = this.flyout.createBlock(this.targetBlock);
getFocusManager().focusNode(this.targetBlock);
return true;

this.startWorkspace_ = this.flyout.targetWorkspace;
this.startWorkspace_.updateScreenCalculationsIfScrolled();
// Start the event group now, so that the same event group is used for
// block creation and block dragging.
if (!eventUtils.getGroup()) {
eventUtils.setGroup(true);
}
return false;
// The start block is no longer relevant, because this is a drag.
this.startBlock = null;
this.targetBlock = this.flyout.createBlock(this.targetBlock);
getFocusManager().focusNode(this.targetBlock);
return true;
}

/**
Expand Down
Loading