feat(modeling): allow label and group movement everywhere, round two

This partially reverts 0c0932d4c6f54181d7f06a626ef109ae7c00dccb.

Closes #1076

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'
 ];

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

test/spec/features/modeling/behavior/FixHoverBehavior.group.bpmn

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<definitions xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:omgdc="http://www.omg.org/spec/DD/20100524/DC" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="sid-38422fae-e03e-43a3-bef4-bd33b32041b2" targetNamespace="http://bpmn.io/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="4.0.0-beta.1">
+  <process id="Process">
+    <task id="Task" />
+    <group id="Group" categoryValueRef="CategoryValue_1heb3m9" />
+  </process>
+  <category id="Category_1g60m2v">
+    <categoryValue id="CategoryValue_1heb3m9" />
+  </category>
+  <bpmndi:BPMNDiagram id="BpmnDiagram_1">
+    <bpmndi:BPMNPlane id="BpmnPlane_1" bpmnElement="Process">
+      <bpmndi:BPMNShape id="Task_di" bpmnElement="Task">
+        <omgdc:Bounds x="264" y="81" width="100" height="80" />
+      </bpmndi:BPMNShape>
+      <bpmndi:BPMNShape id="Group_di" bpmnElement="Group">
+        <omgdc:Bounds x="185" y="200" width="150" height="120" />
+      </bpmndi:BPMNShape>
+    </bpmndi:BPMNPlane>
+  </bpmndi:BPMNDiagram>
+</definitions>

test/spec/features/modeling/behavior/FixHoverBehavior.label.bpmn

@@ -0,0 +1,20 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<definitions xmlns="http://www.omg.org/spec/BPMN/20100524/MODEL" xmlns:bpmndi="http://www.omg.org/spec/BPMN/20100524/DI" xmlns:omgdc="http://www.omg.org/spec/DD/20100524/DC" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" id="sid-38422fae-e03e-43a3-bef4-bd33b32041b2" targetNamespace="http://bpmn.io/bpmn" exporter="bpmn-js (https://demo.bpmn.io)" exporterVersion="4.0.0-beta.1">
+  <process id="Process">
+    <startEvent id="StartEvent" name="A" />
+    <task id="Task" />
+  </process>
+  <bpmndi:BPMNDiagram id="BpmnDiagram_1">
+    <bpmndi:BPMNPlane id="BpmnPlane_1" bpmnElement="Process">
+      <bpmndi:BPMNShape id="StartEvent_di" bpmnElement="StartEvent">
+        <omgdc:Bounds x="156" y="103" width="36" height="36" />
+        <bpmndi:BPMNLabel>
+          <omgdc:Bounds x="171" y="146" width="7" height="14" />
+        </bpmndi:BPMNLabel>
+      </bpmndi:BPMNShape>
+      <bpmndi:BPMNShape id="Task_di" bpmnElement="Task">
+        <omgdc:Bounds x="264" y="81" width="100" height="80" />
+      </bpmndi:BPMNShape>
+    </bpmndi:BPMNPlane>
+  </bpmndi:BPMNDiagram>
+</definitions>

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());
+        }
+      ));
+
+    });
 
   });
 

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