From 055fdf75e1d237d470270f6c10207cfa65800a0c Mon Sep 17 00:00:00 2001 From: Nico Rehwaldt Date: Fri, 14 Jun 2019 13:24:09 +0200 Subject: [PATCH] feat(modeling): allow label and group movement everywhere, round two This partially reverts 0c0932d4c6f54181d7f06a626ef109ae7c00dccb. Closes #1076 --- .../modeling/behavior/FixHoverBehavior.js | 24 +- lib/features/rules/BpmnRules.js | 52 ++--- .../behavior/FixHoverBehavior.group.bpmn | 20 ++ .../behavior/FixHoverBehavior.label.bpmn | 20 ++ .../modeling/behavior/FixHoverBehaviorSpec.js | 213 ++++++++++++++---- test/spec/features/rules/BpmnRulesSpec.js | 14 +- 6 files changed, 243 insertions(+), 100 deletions(-) create mode 100644 test/spec/features/modeling/behavior/FixHoverBehavior.group.bpmn create mode 100644 test/spec/features/modeling/behavior/FixHoverBehavior.label.bpmn diff --git a/lib/features/modeling/behavior/FixHoverBehavior.js b/lib/features/modeling/behavior/FixHoverBehavior.js index 39219699..e56078e1 100644 --- a/lib/features/modeling/behavior/FixHoverBehavior.js +++ b/lib/features/modeling/behavior/FixHoverBehavior.js @@ -7,13 +7,14 @@ import { isAny } from '../util/ModelingUtil'; var HIGH_PRIORITY = 1500; /** - * Ensure we correct hover targets to improve diagram interaction - * during create and move. + * Correct hover targets in certain situations to improve diagram interaction. * * @param {ElementRegistry} elementRegistry * @param {EventBus} eventBus + * @param {Canvas} canvas */ -export default function FixHoverBehavior(elementRegistry, eventBus) { +export default function FixHoverBehavior(elementRegistry, eventBus, canvas) { + eventBus.on([ 'create.hover', 'create.move', @@ -23,17 +24,30 @@ export default function FixHoverBehavior(elementRegistry, eventBus) { 'shape.move.end' ], HIGH_PRIORITY, function(event) { var context = event.context, - shape = context.shape, + shape = context.shape || event.shape, hover = event.hover; + // ensure elements are not dropped onto a bpmn:Lane but onto + // the underlying bpmn:Participant if (is(hover, 'bpmn:Lane') && !isAny(shape, [ 'bpmn:Lane', 'bpmn:Participant' ])) { event.hover = getLanesRoot(hover); event.hoverGfx = elementRegistry.getGraphics(event.hover); } + + var rootElement = canvas.getRootElement(); + + // ensure bpmn:Group and label elements are dropped + // always onto the root + if (hover !== rootElement && (shape.labelTarget || is(shape, 'bpmn:Group'))) { + event.hover = rootElement; + event.hoverGfx = elementRegistry.getGraphics(event.hover); + } }); + } FixHoverBehavior.$inject = [ 'elementRegistry', - 'eventBus' + 'eventBus', + 'canvas' ]; \ No newline at end of file diff --git a/lib/features/rules/BpmnRules.js b/lib/features/rules/BpmnRules.js index 7cf85146..d2e14bb0 100644 --- a/lib/features/rules/BpmnRules.js +++ b/lib/features/rules/BpmnRules.js @@ -123,25 +123,10 @@ BpmnRules.prototype.init = function() { shapes = context.shapes, position = context.position; - var attach = canAttach(shapes, target, null, position); - - if (attach || attach === null) { - return attach; - } - - var replace = canReplace(shapes, target, position); - - if (replace || replace === null) { - return replace; - } - - var move = canMove(shapes, target, position); - - if (move || move === null) { - return move; - } - - return canInsert(shapes, target, position); + return canAttach(shapes, target, null, position) || + canReplace(shapes, target, position) || + canMove(shapes, target, position) || + canInsert(shapes, target, position); }); this.addRule('shape.create', function(context) { @@ -464,11 +449,12 @@ function canConnect(source, target, connection) { */ function canDrop(element, target, position) { - // can move labels everywhere - if (isLabel(element)) { - return null; + // can move labels and groups everywhere + if (isLabel(element) || isGroup(element)) { + return true; } + // disallow to create elements on collapsed pools if (is(target, 'bpmn:Participant') && !isExpanded(target)) { return false; @@ -766,21 +752,9 @@ function canMove(elements, target) { return true; } - var move, - currentMove; - - for (var i = 0; i < elements.length; i++) { - - currentMove = canDrop(elements[i], target); - - if (currentMove === false) { - return false; - } - - move = move || currentMove; - } - - return move; + return elements.every(function(element) { + return canDrop(element, target); + }); } function canCreate(shape, target, source, position) { @@ -789,8 +763,8 @@ function canCreate(shape, target, source, position) { return false; } - if (isLabel(target)) { - return null; + if (isLabel(target) || isGroup(target)) { + return true; } if (isSame(source, target)) { diff --git a/test/spec/features/modeling/behavior/FixHoverBehavior.group.bpmn b/test/spec/features/modeling/behavior/FixHoverBehavior.group.bpmn new file mode 100644 index 00000000..942b6060 --- /dev/null +++ b/test/spec/features/modeling/behavior/FixHoverBehavior.group.bpmn @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/test/spec/features/modeling/behavior/FixHoverBehavior.label.bpmn b/test/spec/features/modeling/behavior/FixHoverBehavior.label.bpmn new file mode 100644 index 00000000..f6c98985 --- /dev/null +++ b/test/spec/features/modeling/behavior/FixHoverBehavior.label.bpmn @@ -0,0 +1,20 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/test/spec/features/modeling/behavior/FixHoverBehaviorSpec.js b/test/spec/features/modeling/behavior/FixHoverBehaviorSpec.js index f1469d5e..ece95836 100644 --- a/test/spec/features/modeling/behavior/FixHoverBehaviorSpec.js +++ b/test/spec/features/modeling/behavior/FixHoverBehaviorSpec.js @@ -11,82 +11,197 @@ import moveModule from 'diagram-js/lib/features/move'; import { createCanvasEvent as canvasEvent } from '../../../../util/MockEvents'; +var testModules = [ + coreModule, + createModule, + moveModule, + modelingModule +]; + describe('features/modeling/behavior - fix hover', function() { - var testModules = [ - coreModule, - createModule, - moveModule, - modelingModule - ]; + describe('drop on lane', function() { - var diagramXML = require('./FixHoverBehavior.participant.bpmn'); + var diagramXML = require('./FixHoverBehavior.participant.bpmn'); - beforeEach(bootstrapModeler(diagramXML, { - modules: testModules - })); + beforeEach(bootstrapModeler(diagramXML, { + modules: testModules + })); - beforeEach(inject(function(dragging) { - dragging.setOptions({ manual: true }); - })); + beforeEach(inject(function(dragging) { + dragging.setOptions({ manual: true }); + })); - var lane, - laneGfx, - participant; + var lane, + laneGfx, + participant; - beforeEach(inject(function(elementRegistry) { - participant = elementRegistry.get('Participant_1'); + beforeEach(inject(function(elementRegistry) { + participant = elementRegistry.get('Participant_1'); - lane = elementRegistry.get('Lane_1'); - laneGfx = elementRegistry.getGraphics(lane); - })); + lane = elementRegistry.get('Lane_1'); + laneGfx = elementRegistry.getGraphics(lane); + })); - describe('create', function() { - it('should ensure hovering participant', inject( - function(create, dragging, elementFactory) { + describe('create', function() { - // given - var task = elementFactory.createShape({ type: 'bpmn:Task' }); + it('should set participant as hover element', inject( + function(create, dragging, elementFactory) { - create.start(canvasEvent({ x: 0, y: 0 }), task, true); + // given + var task = elementFactory.createShape({ type: 'bpmn:Task' }); - // when - dragging.hover({ element: lane, gfx: laneGfx }); + create.start(canvasEvent({ x: 0, y: 0 }), task, true); - dragging.move(canvasEvent({ x: 200, y: 200 })); + // when + dragging.hover({ element: lane, gfx: laneGfx }); - dragging.end(); + dragging.move(canvasEvent({ x: 200, y: 200 })); - // then - expect(task.parent).to.equal(participant); - } - )); + dragging.end(); + + // then + expect(task.parent).to.equal(participant); + } + )); + + }); + + + describe('move', function() { + + it('should set participant as hover element', inject( + function(dragging, elementRegistry, move) { + + // given + var task = elementRegistry.get('Task_1'); + + move.start(canvasEvent({ x: 440, y: 220 }), task, true); + + // when + dragging.hover({ element: lane, gfx: laneGfx }); + + dragging.move(canvasEvent({ x: 240, y: 220 })); + + dragging.end(); + + // then + expect(task.parent).to.equal(participant); + } + )); + + }); }); - describe('move', function() { + describe('label', function() { - it('should ensure hovering participant', inject( - function(dragging, elementRegistry, move) { + var diagramXML = require('./FixHoverBehavior.label.bpmn'); - // given - var task = elementRegistry.get('Task_1'); + beforeEach(bootstrapModeler(diagramXML, { + modules: testModules + })); - move.start(canvasEvent({ x: 440, y: 220 }), task, true); + beforeEach(inject(function(dragging) { + dragging.setOptions({ manual: true }); + })); - // when - dragging.hover({ element: lane, gfx: laneGfx }); - dragging.move(canvasEvent({ x: 240, y: 220 })); + describe('move', function() { - dragging.end(); + it('should set root as hover element', inject( + function(dragging, elementRegistry, move, canvas) { - // then - expect(task.parent).to.equal(participant); - } - )); + // given + var startEvent = elementRegistry.get('StartEvent'); + + var label = startEvent.label; + + move.start(canvasEvent({ x: 175, y: 150 }), label, true); + + // when + dragging.hover({ element: startEvent, gfx: elementRegistry.getGraphics(startEvent) }); + + dragging.move(canvasEvent({ x: 240, y: 220 })); + + dragging.end(); + + // then + expect(label.parent).to.equal(canvas.getRootElement()); + } + )); + + }); + + }); + + + describe('group', function() { + + var diagramXML = require('./FixHoverBehavior.group.bpmn'); + + beforeEach(bootstrapModeler(diagramXML, { + modules: testModules + })); + + beforeEach(inject(function(dragging) { + dragging.setOptions({ manual: true }); + })); + + + describe('create', function() { + + it('should set root as hover element', inject( + function(dragging, elementFactory, elementRegistry, create, canvas) { + + // given + var task = elementRegistry.get('Task'); + + var group = elementFactory.createShape({ type: 'bpmn:Group' }); + + create.start(canvasEvent({ x: 0, y: 0 }), group, true); + + // when + dragging.hover({ element: task, gfx: elementRegistry.getGraphics(task) }); + + dragging.move(canvasEvent({ x: 240, y: 220 })); + + dragging.end(); + + // then + expect(group.parent).to.equal(canvas.getRootElement()); + } + )); + + }); + + + describe('move', function() { + + it('should set root as hover element', inject( + function(dragging, elementRegistry, move, canvas) { + + // given + var task = elementRegistry.get('Task'); + var group = elementRegistry.get('Group'); + + move.start(canvasEvent({ x: 175, y: 150 }), group, true); + + // when + dragging.hover({ element: task, gfx: elementRegistry.getGraphics(task) }); + + dragging.move(canvasEvent({ x: 240, y: 220 })); + + dragging.end(); + + // then + expect(group.parent).to.equal(canvas.getRootElement()); + } + )); + + }); }); diff --git a/test/spec/features/rules/BpmnRulesSpec.js b/test/spec/features/rules/BpmnRulesSpec.js index a69d6358..03a3e9e5 100644 --- a/test/spec/features/rules/BpmnRulesSpec.js +++ b/test/spec/features/rules/BpmnRulesSpec.js @@ -1015,38 +1015,38 @@ describe('features/modeling/rules - BpmnRules', function() { it('-> MessageFlow', function() { - expectCanDrop(label, 'MessageFlow_labeled', null); + expectCanDrop(label, 'MessageFlow_labeled', true); }); it('-> CollapsedParticipant', function() { - expectCanDrop(label, 'CollapsedParticipant', null); + expectCanDrop(label, 'CollapsedParticipant', true); }); it('-> Collaboration', function() { // then - expectCanDrop(label, 'Collaboration', null); + expectCanDrop(label, 'Collaboration', true); }); it('-> Task_in_SubProcess', function() { - expectCanDrop(label, 'Task_in_SubProcess', null); + expectCanDrop(label, 'Task_in_SubProcess', true); }); it('-> SequenceFlow', function() { - expectCanDrop(label, 'SequenceFlow', null); + expectCanDrop(label, 'SequenceFlow', true); }); it('-> DataOutputAssociation', function() { - expectCanDrop(label, 'DataOutputAssociation', null); + expectCanDrop(label, 'DataOutputAssociation', true); }); it('-> Group', function() { - expectCanDrop(label, 'Group', null); + expectCanDrop(label, 'Group', true); }); });