Skip to content
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

Jump to bottom

2210 Fix issue w moving elements beyond border #2244

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

2210 Fix issue w moving elements beyond border #2244

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0b136b2
changed target check in canMove to check for undefined to allow preve…
Adam-Thometz Oct 7, 2024
925bcc7
fix(feature/snapping): changed target check in canMove to check for u…
Adam-Thometz Oct 7, 2024
39169f6
Merge branch '2210-prevent-moving-elements-beyond-border' of https://…
Adam-Thometz Oct 8, 2024
f416dcd
fix(feature/snapping) added test
Adam-Thometz Oct 8, 2024
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
2 changes: 1 addition & 1 deletion lib/features/rules/BpmnRules.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,7 @@ function canMove(elements, target) {
}

// allow default move check to start move operation
if (!target) {
if (target === undefined) {
return true;
}

Expand Down
32 changes: 32 additions & 0 deletions test/spec/features/snapping/BpmnCreateMoveSnappingSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,38 @@ describe('features/snapping - BpmnCreateMoveSnapping', function() {

});

describe('dragging elements', function() {
var diagramXML = require('./BpmnCreateMoveSnapping.process.bpmn');

beforeEach(bootstrapModeler(diagramXML, {
modules: testModules
}));

it('should limit movement when attempting to drag outside the viewport', inject(function(elementRegistry, move, dragging, canvas) {
Copy link
Member

Choose a reason for hiding this comment

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

I've reverted the change to the rules, and the test still passes. What we'd like to achieve is that the test is failing without the change you introduced: https://github.com/bpmn-io/bpmn-js/pull/2244/files#diff-6031dc6e8e3d4e38aef3c1dda35e6ff836a6b6be1263157300284d0191a6d5d4R931
Otherwise, the test does not verify the fix as it is passing without any changes to the library code.

Copy link
Author

Choose a reason for hiding this comment

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

Would it be possible to view some documentation on how the inject function works? Trying to figure out how to simulate the viewport bc in the test it does move outside but it isn't perceived as a viewport so it doesn't snap back if that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

@Adam-Thometz inject will execute the test within a previously bootstrapped bpmn-js instance, and allows you to access internal services.

Similar to this:

const bpmnJS = new BpmnModeler();

const testFn = function(eventBus) { ... };

bpmnJS.invoke(testFn);


var task = elementRegistry.get('Task_1');
var viewbox = canvas.viewbox();
var initialX = task.x;
var initialY = task.y;

// when
move.start(canvasEvent({ x: task.x + task.width / 2, y: task.y + task.height / 2 }), task);

// Attempt to move far outside the viewport
var farAwayX = viewbox.x + viewbox.width * 2;
var farAwayY = viewbox.y + viewbox.height * 2;
dragging.move(canvasEvent({ x: farAwayX, y: farAwayY }));

dragging.end();

// then
expect(task.x).to.be.above(initialX, 'Task should have moved right');
expect(task.y).to.be.above(initialY, 'Task should have moved down');
expect(task.x).to.be.below(farAwayX, 'Task should not have moved as far right as attempted');
expect(task.y).to.be.below(farAwayY, 'Task should not have moved as far down as attempted');
}));
});

});

// helpers //////////
Expand Down