From d8eb7b56bbf185521e1e85092da855b1addb61c4 Mon Sep 17 00:00:00 2001 From: Beka Westberg Date: Tue, 7 Nov 2023 21:12:21 +0000 Subject: [PATCH] fix: make autoclose toggleable for flyouts (#7634) * fix: add basic autoclose toggling support * fix: drag areas being incorrect * fix: blocks getting bumped around when dragged into flyout area * fix: respect always-open flyouts attached to toolboxes * fix: flyout not hiding on ws click * fix: have all flyouts filter for capacity * chore: cleanup * fix: view metrics not respecting flyout * chore: fix change detectors * fix: trashcan not firing close event on click --- core/flyout_base.ts | 38 +++++-- core/flyout_horizontal.ts | 16 +-- core/flyout_vertical.ts | 16 +-- core/metrics_manager.ts | 69 ++++++------- tests/mocha/metrics_test.js | 185 +++++++++++++++++++++++++++-------- tests/mocha/trashcan_test.js | 10 +- 6 files changed, 212 insertions(+), 122 deletions(-) diff --git a/core/flyout_base.ts b/core/flyout_base.ts index 49341d37c..219d69e3e 100644 --- a/core/flyout_base.ts +++ b/core/flyout_base.ts @@ -36,6 +36,7 @@ import {WorkspaceSvg} from './workspace_svg.js'; import * as utilsXml from './utils/xml.js'; import * as Xml from './xml.js'; import * as renderManagement from './render_management.js'; +import {IAutoHideable} from './interfaces/i_autohideable.js'; enum FlyoutItemType { BLOCK = 'block', @@ -45,7 +46,10 @@ enum FlyoutItemType { /** * Class for a flyout. */ -export abstract class Flyout extends DeleteArea implements IFlyout { +export abstract class Flyout + extends DeleteArea + implements IAutoHideable, IFlyout +{ /** * Position the flyout. */ @@ -385,10 +389,8 @@ export abstract class Flyout extends DeleteArea implements IFlyout { this.wheel_, ), ); - if (!this.autoClose) { - this.filterWrapper = this.filterForCapacity.bind(this); - this.targetWorkspace.addChangeListener(this.filterWrapper); - } + this.filterWrapper = this.filterForCapacity.bind(this); + this.targetWorkspace.addChangeListener(this.filterWrapper); // Dragging the flyout up and down. this.boundEvents.push( @@ -414,6 +416,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout { component: this, weight: 1, capabilities: [ + ComponentManager.Capability.AUTOHIDEABLE, ComponentManager.Capability.DELETE_AREA, ComponentManager.Capability.DRAG_TARGET, ], @@ -426,7 +429,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout { */ dispose() { this.hide(); - this.workspace_.getComponentManager().removeComponent(this.id); + this.targetWorkspace.getComponentManager().removeComponent(this.id); for (const event of this.boundEvents) { browserEvents.unbind(event); } @@ -480,6 +483,26 @@ export abstract class Flyout extends DeleteArea implements IFlyout { return this.workspace_; } + /** + * Sets whether this flyout automatically closes when blocks are dragged out, + * the workspace is clicked, etc, or not. + */ + setAutoClose(autoClose: boolean) { + this.autoClose = autoClose; + this.targetWorkspace.recordDragTargets(); + this.targetWorkspace.resizeContents(); + } + + /** Automatically hides the flyout if it is an autoclosing flyout. */ + autoHide(onlyClosePopups: boolean): void { + if ( + !onlyClosePopups && + this.targetWorkspace.getFlyout(true) === this && + this.autoClose + ) + this.hide(); + } + /** * Is the flyout visible? * @@ -504,7 +527,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout { if (!this.autoClose) { // Auto-close flyouts are ignored as drag targets, so only non // auto-close flyouts need to have their drag target updated. - this.workspace_.recordDragTargets(); + this.targetWorkspace.recordDragTargets(); } this.updateDisplay(); } @@ -624,6 +647,7 @@ export abstract class Flyout extends DeleteArea implements IFlyout { // Parse the Array, Node or NodeList into a a list of flyout items. const parsedContent = toolbox.convertFlyoutDefToJsonArray(flyoutDef); + if (!parsedContent.length) return; // No need to show an empty flyout. const flyoutInfo = this.createFlyoutInfo(parsedContent); renderManagement.triggerQueuedRenders(); diff --git a/core/flyout_horizontal.ts b/core/flyout_horizontal.ts index 9d827f4ea..02ac7377a 100644 --- a/core/flyout_horizontal.ts +++ b/core/flyout_horizontal.ts @@ -382,22 +382,10 @@ export class HorizontalFlyout extends Flyout { } } - if ( - this.targetWorkspace!.toolboxPosition === this.toolboxPosition_ && - this.toolboxPosition_ === toolbox.Position.TOP && - !this.targetWorkspace!.getToolbox() - ) { - // This flyout is a simple toolbox. Reposition the workspace so that - // (0,0) is in the correct position relative to the new absolute edge - // (ie toolbox edge). - this.targetWorkspace!.translate( - this.targetWorkspace!.scrollX, - this.targetWorkspace!.scrollY + flyoutHeight, - ); - } this.height_ = flyoutHeight; this.position(); - this.targetWorkspace!.recordDragTargets(); + this.targetWorkspace.resizeContents(); + this.targetWorkspace.recordDragTargets(); } } } diff --git a/core/flyout_vertical.ts b/core/flyout_vertical.ts index b3e656d0c..72c53efbf 100644 --- a/core/flyout_vertical.ts +++ b/core/flyout_vertical.ts @@ -375,22 +375,10 @@ export class VerticalFlyout extends Flyout { } } - if ( - this.targetWorkspace!.toolboxPosition === this.toolboxPosition_ && - this.toolboxPosition_ === toolbox.Position.LEFT && - !this.targetWorkspace!.getToolbox() - ) { - // This flyout is a simple toolbox. Reposition the workspace so that - // (0,0) is in the correct position relative to the new absolute edge - // (ie toolbox edge). - this.targetWorkspace!.translate( - this.targetWorkspace!.scrollX + flyoutWidth, - this.targetWorkspace!.scrollY, - ); - } this.width_ = flyoutWidth; this.position(); - this.targetWorkspace!.recordDragTargets(); + this.targetWorkspace.resizeContents(); + this.targetWorkspace.recordDragTargets(); } } } diff --git a/core/metrics_manager.ts b/core/metrics_manager.ts index cbadcefac..62a2614b6 100644 --- a/core/metrics_manager.ts +++ b/core/metrics_manager.ts @@ -111,26 +111,25 @@ export class MetricsManager implements IMetricsManager { */ getAbsoluteMetrics(): AbsoluteMetrics { let absoluteLeft = 0; + let absoluteTop = 0; + const toolboxMetrics = this.getToolboxMetrics(); - const flyoutMetrics = this.getFlyoutMetrics(true); - const doesToolboxExist = !!this.workspace_.getToolbox(); - const doesFlyoutExist = !!this.workspace_.getFlyout(true); - const toolboxPosition = doesToolboxExist + const flyoutMetrics = this.getFlyoutMetrics(); + const respectToolbox = !!this.workspace_.getToolbox(); + const respectFlyout = !this.workspace_.getFlyout()?.autoClose; + const toolboxPosition = respectToolbox ? toolboxMetrics.position : flyoutMetrics.position; const atLeft = toolboxPosition === toolboxUtils.Position.LEFT; const atTop = toolboxPosition === toolboxUtils.Position.TOP; - if (doesToolboxExist && atLeft) { - absoluteLeft = toolboxMetrics.width; - } else if (doesFlyoutExist && atLeft) { - absoluteLeft = flyoutMetrics.width; + if (atLeft) { + if (respectToolbox) absoluteLeft += toolboxMetrics.width; + if (respectFlyout) absoluteLeft += flyoutMetrics.width; } - let absoluteTop = 0; - if (doesToolboxExist && atTop) { - absoluteTop = toolboxMetrics.height; - } else if (doesFlyoutExist && atTop) { - absoluteTop = flyoutMetrics.height; + if (atTop) { + if (respectToolbox) absoluteTop += toolboxMetrics.height; + if (respectFlyout) absoluteTop += flyoutMetrics.height; } return { @@ -152,36 +151,26 @@ export class MetricsManager implements IMetricsManager { const scale = opt_getWorkspaceCoordinates ? this.workspace_.scale : 1; const svgMetrics = this.getSvgMetrics(); const toolboxMetrics = this.getToolboxMetrics(); - const flyoutMetrics = this.getFlyoutMetrics(true); - const doesToolboxExist = !!this.workspace_.getToolbox(); - const toolboxPosition = doesToolboxExist + const flyoutMetrics = this.getFlyoutMetrics(); + const respectToolbox = !!this.workspace_.getToolbox(); + const respectFlyout = !this.workspace_.getFlyout()?.autoClose; + const toolboxPosition = respectToolbox ? toolboxMetrics.position : flyoutMetrics.position; - if (this.workspace_.getToolbox()) { - if ( - toolboxPosition === toolboxUtils.Position.TOP || - toolboxPosition === toolboxUtils.Position.BOTTOM - ) { - svgMetrics.height -= toolboxMetrics.height; - } else if ( - toolboxPosition === toolboxUtils.Position.LEFT || - toolboxPosition === toolboxUtils.Position.RIGHT - ) { - svgMetrics.width -= toolboxMetrics.width; - } - } else if (this.workspace_.getFlyout(true)) { - if ( - toolboxPosition === toolboxUtils.Position.TOP || - toolboxPosition === toolboxUtils.Position.BOTTOM - ) { - svgMetrics.height -= flyoutMetrics.height; - } else if ( - toolboxPosition === toolboxUtils.Position.LEFT || - toolboxPosition === toolboxUtils.Position.RIGHT - ) { - svgMetrics.width -= flyoutMetrics.width; - } + const horizToolbox = + toolboxPosition === toolboxUtils.Position.TOP || + toolboxPosition === toolboxUtils.Position.BOTTOM; + const vertToolbox = + toolboxPosition === toolboxUtils.Position.LEFT || + toolboxPosition === toolboxUtils.Position.RIGHT; + if (horizToolbox) { + if (respectToolbox) svgMetrics.height -= toolboxMetrics.height; + if (respectFlyout) svgMetrics.height -= flyoutMetrics.height; + } + if (vertToolbox) { + if (respectToolbox) svgMetrics.width -= toolboxMetrics.width; + if (respectFlyout) svgMetrics.width -= flyoutMetrics.width; } return { height: svgMetrics.height / scale, diff --git a/tests/mocha/metrics_test.js b/tests/mocha/metrics_test.js index 83fa605c6..ea2cba7ba 100644 --- a/tests/mocha/metrics_test.js +++ b/tests/mocha/metrics_test.js @@ -68,45 +68,93 @@ suite('Metrics', function () { 'getFlyout', ); }); - test('Toolbox at left', function () { - this.toolboxMetricsStub.returns({width: 107, height: 0, position: 2}); - this.flyoutMetricsStub.returns({}); + + test('left toolboxes with always open flyouts have both offsets', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); this.getToolboxStub.returns(true); - this.getFlyoutStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); - assertDimensionsMatch(absoluteMetrics, 107, 0); + assertDimensionsMatch(absoluteMetrics, 150, 0); }); - test('Toolbox at top', function () { - this.toolboxMetricsStub.returns({width: 0, height: 107, position: 0}); - this.flyoutMetricsStub.returns({}); + + test('top toolboxes with always open flyouts have both offsets', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); this.getToolboxStub.returns(true); - this.getFlyoutStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); - assertDimensionsMatch(absoluteMetrics, 0, 107); + assertDimensionsMatch(absoluteMetrics, 0, 150); }); - test('Flyout at left', function () { - this.toolboxMetricsStub.returns({}); - this.flyoutMetricsStub.returns({width: 107, height: 0, position: 2}); - this.getToolboxStub.returns(false); - this.getFlyoutStub.returns(true); + + test('left toolboxes with autoclosing flyouts only have a toolbox offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); + this.getToolboxStub.returns(true); + this.getFlyoutStub.returns({autoClose: true}); const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); - assertDimensionsMatch(absoluteMetrics, 107, 0); + assertDimensionsMatch(absoluteMetrics, 50, 0); }); - test('Flyout at top', function () { - this.toolboxMetricsStub.returns({}); - this.flyoutMetricsStub.returns({width: 0, height: 107, position: 0}); - this.getToolboxStub.returns(false); - this.getFlyoutStub.returns(true); + + test('top toolboxes with autoclosing flyouts only have a toolbox offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); + this.getToolboxStub.returns(true); + this.getFlyoutStub.returns({autoClose: true}); const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); - assertDimensionsMatch(absoluteMetrics, 0, 107); + assertDimensionsMatch(absoluteMetrics, 0, 50); + }); + + test('left always open flyouts have a flyout offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); + + const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); + + assertDimensionsMatch(absoluteMetrics, 100, 0); + }); + + test('top always open flyouts have a flyout offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); + + const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); + + assertDimensionsMatch(absoluteMetrics, 0, 100); + }); + + test('left autoclosing flyouts have no offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: true}); + + const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); + + assertDimensionsMatch(absoluteMetrics, 0, 0); + }); + + test('top autoclosing flyouts have no offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: true}); + + const absoluteMetrics = this.metricsManager.getAbsoluteMetrics(); + + assertDimensionsMatch(absoluteMetrics, 0, 0); }); }); @@ -132,50 +180,103 @@ suite('Metrics', function () { ); this.svgMetricsStub = sinon.stub(this.metricsManager, 'getSvgMetrics'); }); - test('Toolbox at left', function () { - this.toolboxMetricsStub.returns({width: 107, height: 0, position: 2}); - this.flyoutMetricsStub.returns({}); + + test('left toolboxes with always open flyouts have both offsets', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); this.svgMetricsStub.returns({width: 500, height: 500}); this.getToolboxStub.returns(true); - this.getFlyoutStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); const viewMetrics = this.metricsManager.getViewMetrics(); - assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 393, 500); + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 350, 500); }); - test('Toolbox at top', function () { - this.toolboxMetricsStub.returns({width: 0, height: 107, position: 0}); - this.flyoutMetricsStub.returns({}); + + test('top toolboxes with always open flyouts have both offsets', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); this.svgMetricsStub.returns({width: 500, height: 500}); this.getToolboxStub.returns(true); - this.getFlyoutStub.returns(false); + this.getFlyoutStub.returns({autoClose: false}); const viewMetrics = this.metricsManager.getViewMetrics(); - assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 393); + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 350); }); - test('Flyout at left', function () { - this.toolboxMetricsStub.returns({}); - this.flyoutMetricsStub.returns({width: 107, height: 0, position: 2}); + + test('left toolboxes with autoclosing flyouts only have a toolbox offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); + this.svgMetricsStub.returns({width: 500, height: 500}); + this.getToolboxStub.returns(true); + this.getFlyoutStub.returns({autoClose: true}); + + const viewMetrics = this.metricsManager.getViewMetrics(); + + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 450, 500); + }); + + test('top toolboxes with autoclosing flyouts only have a toolbox offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); + this.svgMetricsStub.returns({width: 500, height: 500}); + this.getToolboxStub.returns(true); + this.getFlyoutStub.returns({autoClose: true}); + + const viewMetrics = this.metricsManager.getViewMetrics(); + + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 450); + }); + + test('left always open flyouts have a flyout offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); this.svgMetricsStub.returns({width: 500, height: 500}); this.getToolboxStub.returns(false); - this.getFlyoutStub.returns(true); + this.getFlyoutStub.returns({autoClose: false}); const viewMetrics = this.metricsManager.getViewMetrics(); - assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 393, 500); + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 400, 500); }); - test('Flyout at top', function () { - this.toolboxMetricsStub.returns({}); - this.flyoutMetricsStub.returns({width: 0, height: 107, position: 0}); + + test('top always open flyouts have a flyout offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); this.svgMetricsStub.returns({width: 500, height: 500}); this.getToolboxStub.returns(false); - this.getFlyoutStub.returns(true); + this.getFlyoutStub.returns({autoClose: false}); const viewMetrics = this.metricsManager.getViewMetrics(); - assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 393); + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 400); }); + + test('left autoclosing flyouts have no offset', function () { + this.toolboxMetricsStub.returns({width: 50, height: 0, position: 2}); + this.flyoutMetricsStub.returns({width: 100, height: 0, position: 2}); + this.svgMetricsStub.returns({width: 500, height: 500}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: true}); + + const viewMetrics = this.metricsManager.getViewMetrics(); + + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 500); + }); + + test('top autoclosing flyouts have no offset', function () { + this.toolboxMetricsStub.returns({width: 0, height: 50, position: 0}); + this.flyoutMetricsStub.returns({width: 0, height: 100, position: 0}); + this.svgMetricsStub.returns({width: 500, height: 500}); + this.getToolboxStub.returns(false); + this.getFlyoutStub.returns({autoClose: true}); + + const viewMetrics = this.metricsManager.getViewMetrics(); + + assertDimensionsMatch(viewMetrics, -SCROLL_X, -SCROLL_Y, 500, 500); + }); + test('Get view metrics in workspace coordinates ', function () { const scale = 2; const getWorkspaceCoordinates = true; diff --git a/tests/mocha/trashcan_test.js b/tests/mocha/trashcan_test.js index fd622c26e..14acd21b6 100644 --- a/tests/mocha/trashcan_test.js +++ b/tests/mocha/trashcan_test.js @@ -121,14 +121,14 @@ suite('Trashcan', function () { }); }); test('Click outside trashcan - fires trashcanClose', function () { - sinon.stub(this.trashcan.flyout, 'isVisible').returns(true); - // Stub flyout interaction. - const hideFlyoutStub = sinon.stub(this.trashcan.flyout, 'hide'); + this.trashcan.flyout.setVisible(true); simulateClick(this.workspace.svgGroup_); - sinon.assert.calledOnce(hideFlyoutStub); - + chai.assert.isFalse( + this.trashcan.flyout.isVisible(), + 'Expected flyout to be hidden', + ); assertEventFired( this.eventsFireStub, Blockly.Events.TrashcanOpen,