Network: Retain constraint values in label font handling (#3520)

* Network: Retain constraint values in label font handling

Fixes #3517.

Due to changed logic in the label font handling, the option values for `widthConstraint` and `heightConstraint` were overwritten.
The fix is in effect a reversal of two code lines: parsing constraint options should come *after* parsing (multi)font options.

Further changes:

- Additional 1-liner fix: constraint values were not copied for edge-instance specific options.
- Small refactoriing of `Label#constrain()` in order to separate concerns
- Added unit test for regression testing of this issue.

This leads to the curious observation that, while the actual change is two lines of source code, this resulted in a +-150 line regression test.

* Made unit test more linear, removed tabs

* Made 'enhanced subset' of unit test

* Removed TODO from comment

* Culled redundant nodes from unit test

lib/network/modules/components/Edge.js

@@ -131,7 +131,8 @@ class Edge {
       'value',
       'width',
       'font',
-      'chosen'
+      'chosen',
+      'widthConstraint'
     ];
 
     // only deep extend the items in the field array. These do not have shorthand.

lib/network/modules/components/shared/Label.js

@@ -144,48 +144,57 @@ class Label {
 
   /**
    * Set the width and height constraints based on 'nearest' value
+   *
    * @param {Array} pile array of option objects to consider
+   * @returns {object} the actual constraint values to use
    * @private
    */
   constrain(pile) {
-    this.fontOptions.constrainWidth = false;
-    this.fontOptions.maxWdt = -1;
-    this.fontOptions.minWdt = -1;
+    // NOTE: constrainWidth and  constrainHeight never set!
+    // NOTE: for edge labels, only 'maxWdt' set
+    // Node labels can set all the fields
+    let fontOptions = {
+      constrainWidth: false,
+      maxWdt: -1,
+      minWdt: -1,
+      constrainHeight: false,
+      minHgt: -1,
+      valign: 'middle',
+    }
 
     let widthConstraint = util.topMost(pile, 'widthConstraint');
     if (typeof widthConstraint === 'number') {
-      this.fontOptions.maxWdt = Number(widthConstraint);
-      this.fontOptions.minWdt = Number(widthConstraint);
+      fontOptions.maxWdt = Number(widthConstraint);
+      fontOptions.minWdt = Number(widthConstraint);
     } else if (typeof widthConstraint === 'object') {
       let widthConstraintMaximum = util.topMost(pile, ['widthConstraint', 'maximum']);
       if (typeof widthConstraintMaximum === 'number') {
-        this.fontOptions.maxWdt = Number(widthConstraintMaximum);
+        fontOptions.maxWdt = Number(widthConstraintMaximum);
       }
       let widthConstraintMinimum = util.topMost(pile, ['widthConstraint', 'minimum'])
       if (typeof widthConstraintMinimum === 'number') {
-        this.fontOptions.minWdt = Number(widthConstraintMinimum);
+        fontOptions.minWdt = Number(widthConstraintMinimum);
       }
     }
 
-    this.fontOptions.constrainHeight = false;
-    this.fontOptions.minHgt = -1;
-    this.fontOptions.valign = 'middle';
 
     let heightConstraint = util.topMost(pile, 'heightConstraint');
     if (typeof heightConstraint === 'number') {
-      this.fontOptions.minHgt = Number(heightConstraint);
+      fontOptions.minHgt = Number(heightConstraint);
     } else if (typeof heightConstraint === 'object') {
       let heightConstraintMinimum = util.topMost(pile, ['heightConstraint', 'minimum']);
       if (typeof heightConstraintMinimum === 'number') {
-        this.fontOptions.minHgt = Number(heightConstraintMinimum);
+        fontOptions.minHgt = Number(heightConstraintMinimum);
       }
       let heightConstraintValign = util.topMost(pile, ['heightConstraint', 'valign']);
       if (typeof heightConstraintValign === 'string') {
-        if ((heightConstraintValign === 'top')||(heightConstraintValign === 'bottom')) {
-            this.fontOptions.valign = heightConstraintValign;
+        if ((heightConstraintValign === 'top')|| (heightConstraintValign === 'bottom')) {
+          fontOptions.valign = heightConstraintValign;
         }
       }
     }
+
+    return fontOptions;
   }
 
 
@@ -197,8 +206,8 @@ class Label {
    */
   update(options, pile) {
     this.setOptions(options, true);
-    this.constrain(pile);
     this.propagateFonts(pile);
+    util.deepExtend(this.fontOptions, this.constrain(pile));
     this.fontOptions.chooser = ComponentUtil.choosify('label', pile);
   }
 

test/Label.test.js

@@ -32,7 +32,7 @@ class DummyContext {
 
 
 class DummyLayoutEngine {
-	positionInitially() {}
+  positionInitially() {}
 }
 
 /**************************************************************
@@ -72,7 +72,7 @@ describe('Network Label', function() {
     let showBlocks = () => {
       return '\nreturned: ' + JSON.stringify(returned, null, 2) + '\n' +
                'expected: ' + JSON.stringify(expected, null, 2);
-		}
+    }
 
     assert.equal(expected.lines.length, returned.lines.length, 'Number of lines does not match, ' + showBlocks());
 
@@ -121,7 +121,7 @@ describe('Network Label', function() {
     "OnereallylongwordthatshouldgooverwidthConstraint.maximumifdefined",
     "One really long sentence that should go over widthConstraint.maximum if defined",
     "Reallyoneenormouslylargelabel withtwobigwordsgoingoverwayovermax"
-	]
+  ]
 
   var html_text = [
     "label <b>with</b> <code>some</code> <i>multi <b>tags</b></i>",
@@ -917,7 +917,7 @@ describe('Shorthand Font Options', function() {
         }
       }
     });
-	}
+  }
 
 
   function dynamicAdd2(network, data) {
@@ -926,7 +926,7 @@ describe('Shorthand Font Options', function() {
         font: '7 Font7 #070707'  // Note: this kills the font.multi, bold and ital settings!
       }
     });
-	}
+  }
 
 
   it('deals with dynamic data and option updates for shorthand', function (done) {
@@ -1408,6 +1408,150 @@ describe('Shorthand Font Options', function() {
   });
 
 
+  /**
+   * 
+   * The test network is derived from example `network/nodeStyles/widthHeight.html`,
+   * where the associated issue (i.e. widthConstraint values not copied) was most poignant.
+   *
+   * NOTE: boolean shorthand values for widthConstraint and heightConstraint do nothing.
+   */
+  it('Sets the width/height constraints in the font label options', function (done) {
+    var nodes = [
+      { id: 100, label: 'node 100'},
+      { id: 210, group: 'group1', label: 'node 210'},
+      { id: 211, widthConstraint: { minimum: 120 }, label: 'node 211'},
+      { id: 212, widthConstraint: { minimum: 120, maximum: 140 }, group: 'group1', label: 'node 212'},  // group override
+      { id: 220, widthConstraint: { maximum: 170 }, label: 'node 220'},
+      { id: 200, font: { multi: true }, widthConstraint: 150, label: 'node <b>200</b>'},
+      { id: 201, widthConstraint: 150, label: 'node 201'},
+      { id: 202, group: 'group2', label: 'node 202'},
+      { id: 203, heightConstraint: { minimum: 75, valign: 'bottom'}, group: 'group2', label: 'node 203'},  // group override
+      { id: 204, heightConstraint: 80, group: 'group2', label: 'node 204'}, // group override
+      { id: 300, heightConstraint: { minimum: 70 }, label: 'node 300'},
+      { id: 400, heightConstraint: { minimum: 100, valign: 'top' }, label: 'node 400'},
+      { id: 401, heightConstraint: { minimum: 100, valign: 'middle' }, label: 'node 401'},
+      { id: 402, heightConstraint: { minimum: 100, valign: 'bottom' }, label: 'node 402'}
+    ];
+  
+    var edges = [
+      { id: 1, from: 100, to: 210, label: "edge 1"},
+      { id: 2, widthConstraint: 80, from: 210, to: 211, label: "edge 2"},
+      { id: 3, heightConstraint: 90, from: 100, to: 220, label: "edge 3"},
+      { id: 4, from: 401, to: 402, widthConstraint: { maximum: 150 }, label: "edge 12"},
+    ];
+  
+    var container = document.getElementById('mynetwork');
+    var data = {
+      nodes: nodes,
+      edges: edges
+    };
+    var options = {
+      edges: {
+        font: {
+          size: 12
+        },
+        widthConstraint: {
+          maximum: 90
+        }
+      },
+      nodes: {
+        shape: 'box',
+        margin: 10,
+        widthConstraint: {
+          maximum: 200
+        }
+      },
+      groups: {
+        group1: {
+          shape: 'dot',
+          widthConstraint: {
+            maximum: 130
+          }
+        },
+        // Following group serves to test all font options
+        group2: {
+          shape: 'dot',
+          widthConstraint: {
+            minimum: 150,
+            maximum: 180,
+          },
+          heightConstraint: {
+            minimum: 210,
+            valign: 'top',
+          }
+        },
+      },
+      physics: {
+        enabled: false
+      }
+    };
+    var network = new vis.Network(container, data, options);
+
+    var nodes_expected = [
+      { nodeId: 100, minWdt:  -1, maxWdt: 200, minHgt:  -1, valign: 'middle'},
+      { nodeId: 210, minWdt:  -1, maxWdt: 130, minHgt:  -1, valign: 'middle'},
+      { nodeId: 211, minWdt: 120, maxWdt: 200, minHgt:  -1, valign: 'middle'},
+      { nodeId: 212, minWdt: 120, maxWdt: 140, minHgt:  -1, valign: 'middle'},
+      { nodeId: 220, minWdt:  -1, maxWdt: 170, minHgt:  -1, valign: 'middle'},
+      { nodeId: 200, minWdt: 150, maxWdt: 150, minHgt:  -1, valign: 'middle'},
+      { nodeId: 201, minWdt: 150, maxWdt: 150, minHgt:  -1, valign: 'middle'},
+      { nodeId: 202, minWdt: 150, maxWdt: 180, minHgt: 210, valign: 'top'},
+      { nodeId: 203, minWdt: 150, maxWdt: 180, minHgt:  75, valign: 'bottom'},
+      { nodeId: 204, minWdt: 150, maxWdt: 180, minHgt:  80, valign: 'middle'},
+      { nodeId: 300, minWdt:  -1, maxWdt: 200, minHgt:  70, valign: 'middle'},
+      { nodeId: 400, minWdt:  -1, maxWdt: 200, minHgt: 100, valign: 'top'},
+      { nodeId: 401, minWdt:  -1, maxWdt: 200, minHgt: 100, valign: 'middle'},
+      { nodeId: 402, minWdt:  -1, maxWdt: 200, minHgt: 100, valign: 'bottom'},
+    ];
+
+
+    // For edge labels, only maxWdt is set. We check the rest anyway, be it for
+    // checking incorrect settings or for future code changes.
+    //
+    // There is a lot of repetitiveness here. Perhaps using a direct copy of the
+    // example should be let go.
+    var edges_expected = [
+      { id: 1, minWdt: -1, maxWdt:  90, minHgt: -1, valign: 'middle'},
+      { id: 2, minWdt: 80, maxWdt:  80, minHgt: -1, valign: 'middle'},
+      { id: 3, minWdt: -1, maxWdt:  90, minHgt: 90, valign: 'middle'},
+      { id: 4, minWdt: -1, maxWdt: 150, minHgt: -1, valign: 'middle'},
+    ];
+
+
+    let assertConstraints = (expected, fontOptions, label) => {
+      assert.equal(expected.minWdt, fontOptions.minWdt, 'Incorrect min width' + label);
+      assert.equal(expected.maxWdt, fontOptions.maxWdt, 'Incorrect max width' + label);
+      assert.equal(expected.minHgt, fontOptions.minHgt, 'Incorrect min height' + label);
+      assert.equal(expected.valign, fontOptions.valign, 'Incorrect valign' + label);
+    }
+
+
+    // Check nodes
+    util.forEach(nodes_expected, function(expected) {
+      let networkNode = network.body.nodes[expected.nodeId];
+      assert(networkNode !== undefined && networkNode !== null, 'node not found for id: ' + expected.nodeId);
+      let fontOptions = networkNode.labelModule.fontOptions;
+
+      var label = ' for node id: ' + expected.nodeId;
+      assertConstraints(expected, fontOptions, label);
+    });
+
+
+    // Check edges
+    util.forEach(edges_expected, function(expected) {
+      let networkEdge = network.body.edges[expected.id];
+
+      var label = ' for edge id: ' + expected.id;
+      assert(networkEdge !== undefined, 'Edge not found' + label);
+
+      let fontOptions = networkEdge.labelModule.fontOptions;
+      assertConstraints(expected, fontOptions, label);
+    });
+
+    done();
+  });
+
+
   it('deals with null labels and other awkward values', function (done) {
     var ctx = new DummyContext();
     var options = getOptions({});