cleanup function to be more readable (#4756)

* cleanup function to be more readable * update style variable to be more descriptive and use the styleObject reference in one more place
fabricjs · Feb 21, 2018 · 71b2801 · 71b2801
1 parent dfb5cb7
commit 71b2801
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 14 deletions.
diff --git a/src/mixins/text_style.mixin.js b/src/mixins/text_style.mixin.js
@@ -63,31 +63,42 @@
       if (!this.styles || !property || property === '') {
         return false;
       }
-      var obj = this.styles, stylesCount = 0, letterCount, foundStyle = false, style,
-          canBeSwapped = true, graphemeCount = 0;
+      var obj = this.styles, stylesCount = 0, letterCount, stylePropertyValue,
+          allStyleObjectPropertiesMatch = true, graphemeCount = 0, styleObject;
       // eslint-disable-next-line
       for (var p1 in obj) {
         letterCount = 0;
         // eslint-disable-next-line
         for (var p2 in obj[p1]) {
+          var styleObject = obj[p1][p2],
+              stylePropertyHasBeenSet = styleObject.hasOwnProperty(property);
+
           stylesCount++;
-          if (!foundStyle) {
-            style = obj[p1][p2][property];
-            foundStyle = true;
-          }
-          else if (obj[p1][p2][property] !== style || !obj[p1][p2].hasOwnProperty(property)) {
-            canBeSwapped = false;
+
+          if (stylePropertyHasBeenSet) {
+            if (!stylePropertyValue) {
+              stylePropertyValue = styleObject[property];
+            }
+            else if (styleObject[property] !== stylePropertyValue) {
+              allStyleObjectPropertiesMatch = false;
+            }
+
+            if (styleObject[property] === this[property]) {
+              delete styleObject[property];
+            }
           }
-          if (obj[p1][p2][property] === this[property]) {
-            delete obj[p1][p2][property];
+          else {
+            allStyleObjectPropertiesMatch = false;
           }
-          if (Object.keys(obj[p1][p2]).length !== 0) {
+
+          if (Object.keys(styleObject).length !== 0) {
             letterCount++;
           }
           else {
             delete obj[p1][p2];
           }
         }
+
         if (letterCount === 0) {
           delete obj[p1];
         }
@@ -97,8 +108,8 @@
       for (var i = 0; i < this._textLines.length; i++) {
         graphemeCount += this._textLines[i].length;
       }
-      if (canBeSwapped && stylesCount === graphemeCount) {
-        this[property] = style;
+      if (allStyleObjectPropertiesMatch && stylesCount === graphemeCount) {
+        this[property] = stylePropertyValue;
         this.removeStyle(property);
       }
     },

diff --git a/test/unit/text.js b/test/unit/text.js
@@ -338,13 +338,14 @@
 
   QUnit.test('text cleanStyle with different sub styles styles', function(assert) {
     var text = new fabric.Text('xxxxxx\nx y');
-    text.styles = { 1: { 0: { fill: 'red' }, 1: { stroke: 'red' }}};
+    text.styles = { 1: { 0: { fill: 'red' }, 1: { stroke: 'red' }, 2: { stroke: 'blue' }}};
     text.stroke = 'red';
     text.cleanStyle('stroke');
     assert.equal(text.stroke, 'red', 'the stroke stays red');
     assert.equal(text.styles[1][0].fill, 'red', 'the style has not been changed since it\'s a different property');
     assert.equal(text.styles[1][0].stroke, undefined, 'the style has been cleaned since stroke was equal to text property');
     assert.equal(text.styles[1][1], undefined, 'the style remains undefined');
+    assert.equal(text.styles[1][2].stroke, 'blue', 'the style remains unchanged');
   });
 
   QUnit.test('text cleanStyle with undefined and set styles', function(assert) {