fix(label-support): fix positioning of label created on shape create

* make sure label shape is created with correct size
* make sure test varifies behaviour

Closes camunda/camunda-modeler#535

lib/features/modeling/behavior/LabelBehavior.js

@@ -15,6 +15,13 @@ var hasExternalLabel = LabelUtil.hasExternalLabel,
 
 var CommandInterceptor = require('diagram-js/lib/command/CommandInterceptor');
 
+var TextUtil = require('diagram-js/lib/util/Text');
+
+var DEFAULT_LABEL_DIMENSIONS = {
+  width: 90,
+  height: 20
+};
+
 
 /**
  * A component that makes sure that external labels are added
@@ -29,6 +36,8 @@ function LabelSupport(eventBus, modeling, bpmnFactory) {
 
   CommandInterceptor.call(this, eventBus);
 
+  var textUtil = new TextUtil();
+
 
   ///// create external labels on shape creation
 
@@ -38,17 +47,26 @@ function LabelSupport(eventBus, modeling, bpmnFactory) {
     var element = context.shape || context.connection,
         businessObject = element.businessObject;
 
-    var position;
+    if (!hasExternalLabel(businessObject)) {
-
+      return;
-    if (hasExternalLabel(businessObject)) {
-      position = getExternalLabelMid(element);
-
-      modeling.createLabel(element, position, {
-        id: businessObject.id + '_label',
-        hidden: !businessObject.name,
-        businessObject: businessObject
-      });
     }
+
+    var labelCenter = getExternalLabelMid(element);
+
+    // we don't care about x and y
+    var labelDimensions = getLayoutedBounds(
+      DEFAULT_LABEL_DIMENSIONS,
+      businessObject.name || '',
+      textUtil
+    );
+
+    modeling.createLabel(element, labelCenter, {
+      id: businessObject.id + '_label',
+      hidden: !businessObject.name,
+      businessObject: businessObject,
+      width: labelDimensions.width,
+      height: labelDimensions.height
+    });
   });
 
 
@@ -173,3 +191,32 @@ inherits(LabelSupport, CommandInterceptor);
 LabelSupport.$inject = [ 'eventBus', 'modeling', 'bpmnFactory' ];
 
 module.exports = LabelSupport;
+
+
+// TODO(nikku): repeating code (search for <getLayoutedBounds>)
+
+var EXTERNAL_LABEL_STYLE = {
+  fontFamily: 'Arial, sans-serif',
+  fontSize: '11px'
+};
+
+function getLayoutedBounds(bounds, text, textUtil) {
+
+  var layoutedLabelDimensions = textUtil.getDimensions(text, {
+    box: {
+      width: 90,
+      height: 30,
+      x: bounds.width / 2 + bounds.x,
+      y: bounds.height / 2 + bounds.y
+    },
+    style: EXTERNAL_LABEL_STYLE
+  });
+
+  // resize label shape to fit label text
+  return {
+    x: Math.round(bounds.x + bounds.width / 2 - layoutedLabelDimensions.width / 2),
+    y: Math.round(bounds.y),
+    width: Math.ceil(layoutedLabelDimensions.width),
+    height: Math.ceil(layoutedLabelDimensions.height)
+  };
+}

test/spec/features/modeling/LabelLayoutingSpec.js

@@ -24,7 +24,7 @@ var testModules = [
 
 describe('modeling - label layouting', function() {
 
-  describe('should create label', function() {
+  describe('should position created label', function() {
 
     var diagramXML = require('./LabelLayouting.initial.bpmn');
 
@@ -33,7 +33,7 @@ describe('modeling - label layouting', function() {
     }));
 
 
-    it('horizontal', inject(function(modeling, elementRegistry) { // only
+    it('horizontal', inject(function(modeling, elementRegistry) {
 
       // given
       var element1 = elementRegistry.get('StartEvent_1'),
@@ -43,10 +43,8 @@ describe('modeling - label layouting', function() {
       var connection = modeling.connect(element1, element2);
 
       // then
-      // we don't need to resize the label since it's invisible
+      expect(connection.label.x).to.be.equal(472);
-      // therefore it has the standard width of 90px
+      expect(connection.label.y).to.be.within(335, 336);
-      expect(connection.label.x).to.be.equal(427);
-      expect(connection.label.y).to.be.equal(332);
     }));
 
 
@@ -60,8 +58,8 @@ describe('modeling - label layouting', function() {
       var connection = modeling.connect(element1, element2);
 
       // then
-      expect(connection.label.x).to.be.equal(292);
+      expect(connection.label.x).to.be.equal(337);
-      expect(connection.label.y).to.be.equal(219.5);
+      expect(connection.label.y).to.be.within(222, 224);
     }));
 
   });

test/spec/features/modeling/behavior/LabelBehaviorSpec.js

@@ -4,8 +4,6 @@ require('../../../../TestHelper');
 
 /* global bootstrapModeler, inject */
 
-var assign = require('lodash/object/assign');
-
 var modelingModule = require('../../../../../lib/features/modeling'),
     coreModule = require('../../../../../lib/core');
 
@@ -81,7 +79,10 @@ describe('behavior - LabelBehavior', function() {
         expect(label).to.exist;
         expect(elementRegistry.get(label.id)).to.exist;
 
-        expect(label).to.have.bounds(assign({ x: 262, y: 138, width: 90, height: 20 }));
+        expect(label.x).to.equal(307);
+        expect(label.y).to.be.within(141, 142);
+        expect(label.width).to.be.equal(0);
+        expect(label.height).to.be.within(12, 14);
       }));
 
 

test/spec/features/replace/BpmnReplaceSpec.js

@@ -263,15 +263,15 @@ describe('features/replace - bpmn replace', function() {
     it('should keep label position', inject(function(elementRegistry, bpmnReplace, modeling) {
 
       // given
-      var exclusiveGateway = elementRegistry.get('ExclusiveGateway_1');
+      var startEvent = elementRegistry.get('StartEvent_1');
-      var label = elementRegistry.get('ExclusiveGateway_1_label');
+      var label = elementRegistry.get('StartEvent_1_label');
 
       var newElementData =  {
-        type: 'bpmn:InclusiveGateway'
+        type: 'bpmn:EndEvent'
       };
 
       // when
-      var newElement = bpmnReplace.replaceElement(exclusiveGateway, newElementData);
+      var newElement = bpmnReplace.replaceElement(startEvent, newElementData);
 
       // then
       expect(newElement.label.x).to.equal(label.x);