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.

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

2210 Fix issue w moving elements beyond border #2244

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

Conversation

Adam-Thometz
Copy link

See original issue for context

Proposed Changes

Change one line of code in lib/features/rules/BpmnRules.js in the canMove function:

- if (!target)
+ if (target === undefined)

Reasoning

If a user starts dragging an element, the target is undefined. If a user drags an element outside the canvas, the target is null. Since both cases are considered nullish, they will both return true (meaning the user can drop the element). We want the latter case to return false.

Steps to reproduce

bpmn-demo

  1. Wrap elements in a pool
  2. Try dragging an element within the pool.
    a. If dragged within, should be successful
    b. If dragged outside the element but within the canvas, should fail
    c. If dragged outside the element AND outside the canvas, should fail

@CLAassistant
Copy link

CLAassistant commented Oct 7, 2024

CLA assistant check
All committers have signed the CLA.

@barmac
Copy link
Member

barmac commented Oct 7, 2024

Hi,

Thank you for your PR. Can you please make sure the contribution meets the requirements in CONTRIBUTING?
Specifically, I'd like you to provide a test case to verify the issue is fixed.
The commit message should also adhere to conventional commits guidelines, and end with Closes #issueNumber.

@Adam-Thometz
Copy link
Author

Thank you for the feedback @barmac I updated the commit message and added a test to BpmnCreateMoveSnappingSpec.js

@barmac barmac self-requested a review October 10, 2024 08:58
@barmac
Copy link
Member

barmac commented Oct 10, 2024

Thanks, I will have a look at this again today.

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);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants