refactor(Path, Polyline): BREAKING Svg import - cleanup and path/poly…

…gon positioning (#8857)

CHANGELOG.md

@@ -2,17 +2,19 @@
 
 ## [next]
 
+- refactor(fabric.Line): Line position is calculated from the center between the 2 points now [#8877](https://github.com/fabricjs/fabric.js/pull/8877)
+- chore(Path, Polyline): Clean up old SVG import code [#8857](https://github.com/fabricjs/fabric.js/pull/8857)
+
 ## [6.0.0-beta5]
 
-- refactor(fabric.Line): Line position is calculated from the center between the 2 points now [#8877](https://github.com/fabricjs/fabric.js/pull/8877)
 - bundle(): export `setEnv` for JEST interoperability [#8888](https://github.com/fabricjs/fabric.js/pull/8888)
 
 ## [6.0.0-beta4]
 
 - chore(): Code cleanup and reuse of code in svg-parsing code [#8881](https://github.com/fabricjs/fabric.js/pull/8881)
 - chore(TS): Parse transform attribute typing [#8878](https://github.com/fabricjs/fabric.js/pull/8878)
 - chore(TS): Fix typing for DOMParser [#8871](https://github.com/fabricjs/fabric.js/pull/8871)
-- fix(Path, polyline): fix for SVG import [#8879](https://github.com/fabricjs/fabric.js/pull/8879)
+- fix(Path, Polyline): fix for SVG import [#8879](https://github.com/fabricjs/fabric.js/pull/8879)
 - chore(TS) add types for loadSVGFromURl, parseSVGDocument, loadSVGFromString [#8869](https://github.com/fabricjs/fabric.js/pull/8869)
 - chore(TS): finalize Path migration [#8438](https://github.com/fabricjs/fabric.js/pull/8438)
 - fix(Path, Obect) Fix path parsing edge case for zeroed arc command and for too small canvas patterns [#8853](https://github.com/fabricjs/fabric.js/pull/8853)

src/parser/elements_parser.ts

@@ -107,7 +107,7 @@ const ElementsParser = function (
     }
   };
 
-  proto.createClipPathCallback = function (obj, container) {
+  proto.createClipPathCallback = function (container) {
     return function (_newObj) {
       removeTransformMatrixForSvgParsing(_newObj);
       _newObj.fillRule = _newObj.clipRule;
@@ -116,65 +116,61 @@ const ElementsParser = function (
   };
 
   proto.resolveClipPath = function (obj, usingElement) {
-    let clipPath = this.extractPropertyDefinition(obj, 'clipPath', 'clipPaths'),
-      element,
-      klass,
-      objTransformInv,
-      container,
-      gTransform,
-      options;
-    if (clipPath) {
-      container = [];
-      objTransformInv = invertTransform(obj.calcTransformMatrix());
-      // move the clipPath tag as sibling to the real element that is using it
-      const clipPathTag = clipPath[0].parentNode;
-      let clipPathOwner = usingElement;
-      while (
-        clipPathOwner.parentNode &&
-        clipPathOwner.getAttribute('clip-path') !== obj.clipPath
-      ) {
-        clipPathOwner = clipPathOwner.parentNode;
-      }
-      clipPathOwner.parentNode.appendChild(clipPathTag);
-      for (let i = 0; i < clipPath.length; i++) {
-        element = clipPath[i];
-        klass = this.findTag(element);
-        klass.fromElement(
-          element,
-          this.createClipPathCallback(obj, container),
-          this.options
-        );
-      }
-      if (container.length === 1) {
-        clipPath = container[0];
-      } else {
-        clipPath = new Group(container);
-      }
-      gTransform = multiplyTransformMatrices(
-        objTransformInv,
-        clipPath.calcTransformMatrix()
-      );
-      if (clipPath.clipPath) {
-        this.resolveClipPath(clipPath, clipPathOwner);
-      }
-      const options = qrDecompose(gTransform);
-      clipPath.flipX = false;
-      clipPath.flipY = false;
-      clipPath.set('scaleX', options.scaleX);
-      clipPath.set('scaleY', options.scaleY);
-      clipPath.angle = options.angle;
-      clipPath.skewX = options.skewX;
-      clipPath.skewY = 0;
-      clipPath.setPositionByOrigin(
-        { x: options.translateX, y: options.translateY },
-        'center',
-        'center'
-      );
-      obj.clipPath = clipPath;
-    } else {
+    let clipPath = this.extractPropertyDefinition(obj, 'clipPath', 'clipPaths');
+    if (!clipPath) {
       // if clip-path does not resolve to any element, delete the property.
       delete obj.clipPath;
+      return;
+    }
+    const container = [];
+    const objTransformInv = invertTransform(obj.calcTransformMatrix());
+    // move the clipPath tag as sibling to the real element that is using it
+    const clipPathTag = clipPath[0].parentNode;
+    let clipPathOwner = usingElement;
+    while (
+      clipPathOwner.parentNode &&
+      clipPathOwner.getAttribute('clip-path') !== obj.clipPath
+    ) {
+      clipPathOwner = clipPathOwner.parentNode;
     }
+    clipPathOwner.parentNode.appendChild(clipPathTag);
+    for (let i = 0; i < clipPath.length; i++) {
+      const element = clipPath[i];
+      this.findTag(element).fromElement(
+        element,
+        this.createClipPathCallback(container),
+        this.options
+      );
+    }
+    if (container.length === 1) {
+      clipPath = container[0];
+    } else {
+      clipPath = new Group(container);
+    }
+    const gTransform = multiplyTransformMatrices(
+      objTransformInv,
+      clipPath.calcTransformMatrix()
+    );
+    if (clipPath.clipPath) {
+      this.resolveClipPath(clipPath, clipPathOwner);
+    }
+    const { scaleX, scaleY, angle, skewX, translateX, translateY } =
+      qrDecompose(gTransform);
+    clipPath.set({
+      flipX: false,
+      flipY: false,
+      scaleX,
+      scaleY,
+      angle,
+      skewX,
+      skewY: 0,
+    });
+    clipPath.setPositionByOrigin(
+      { x: translateX, y: translateY },
+      'center',
+      'center'
+    );
+    obj.clipPath = clipPath;
   };
 
   proto.checkIfDone = function () {

src/shapes/Path.ts

@@ -57,8 +57,6 @@ export class Path<
 
   declare pathOffset: Point;
 
-  declare fromSVG?: boolean;
-
   declare sourcePath?: string;
 
   declare segmentsInfo?: TPathSegmentInfo[];
@@ -293,10 +291,11 @@ export class Path<
   }
 
   setBoundingBox(adjustPosition?: boolean) {
-    const { left, top, width, height, pathOffset } = this._calcDimensions();
+    const { width, height, pathOffset } = this._calcDimensions();
     this.set({ width, height, pathOffset });
-    adjustPosition &&
-      this.setPositionByOrigin(new Point(left, top), 'left', 'top');
+    // using pathOffset because it match the use case.
+    // if pathOffset change here we need to use left + width/2 , top + height/2
+    adjustPosition && this.setPositionByOrigin(pathOffset, 'center', 'center');
   }
 
   _calcBoundsFromPath(): TBBox {
@@ -372,12 +371,9 @@ export class Path<
    */
   _calcDimensions(): IPathBBox {
     const bbox = this._calcBoundsFromPath();
-    const strokeCorrection = this.fromSVG ? 0 : this.strokeWidth / 2;
 
     return {
       ...bbox,
-      left: bbox.left - strokeCorrection,
-      top: bbox.top - strokeCorrection,
       pathOffset: new Point(
         bbox.left + bbox.width / 2,
         bbox.top + bbox.height / 2
@@ -427,7 +423,6 @@ export class Path<
         // we pass undefined to instruct the constructor to position the object using the bbox
         left: undefined,
         top: undefined,
-        fromSVG: true,
       })
     );
   }

src/shapes/Polyline.ts

@@ -74,8 +74,6 @@ export class Polyline<
     'points',
   ];
 
-  declare fromSVG: boolean;
-
   declare pathOffset: Point;
 
   declare strokeOffset: Point;
@@ -146,13 +144,8 @@ export class Polyline<
       offsetX - offsetY * Math.tan(degreesToRadians(this.skewX));
     const pathOffsetY =
       offsetY - pathOffsetX * Math.tan(degreesToRadians(this.skewY));
-    // TODO: remove next line
-    const legacyCorrection =
-      !this.fromSVG && !this.exactBoundingBox ? this.strokeWidth / 2 : 0;
     return {
       ...bbox,
-      left: bbox.left - legacyCorrection,
-      top: bbox.top - legacyCorrection,
       pathOffset: new Point(pathOffsetX, pathOffsetY),
       strokeOffset: new Point(bboxNoStroke.left, bboxNoStroke.top).subtract(
         new Point(bbox.left, bbox.top)
@@ -180,7 +173,11 @@ export class Polyline<
       this._calcDimensions();
     this.set({ width, height, pathOffset, strokeOffset });
     adjustPosition &&
-      this.setPositionByOrigin(new Point(left, top), 'left', 'top');
+      this.setPositionByOrigin(
+        new Point(left + width / 2, top + height / 2),
+        'center',
+        'center'
+      );
   }
 
   /**
@@ -345,7 +342,6 @@ export class Polyline<
       new this(points || [], {
         ...parsedAttributes,
         ...options,
-        fromSVG: true,
       })
     );
   }

test/unit/path.js

@@ -11,7 +11,7 @@
     height:                   200,
     fill:                     'red',
     stroke:                   'blue',
-    strokeWidth:              1,
+    strokeWidth:              0,
     strokeDashArray:          null,
     strokeLineCap:            'butt',
     strokeDashOffset:         0,
@@ -48,12 +48,20 @@
     return el;
   }
 
-  function getPathObject(path, callback) {
+  function getPathObjectFromElement(path, callback) {
     fabric.Path.fromElement(getPathElement(path), callback);
   }
 
   function makePathObject(callback) {
-    getPathObject('M 100 100 L 300 100 L 200 300 z', callback);
+    const path = new fabric.Path('M 100 100 L 300 100 L 200 300 z', {
+      fill: 'red',
+      stroke: 'blue',
+      strokeLineCap: 'butt',
+      strokeLineJoin: 'miter',
+      strokeMiterLimit: 4,
+      strokeWidth: 0,
+    });
+    callback(path);
   }
 
   function updatePath(pathObject, value, preservePosition) {
@@ -109,18 +117,42 @@
     done();
   });
 
-  QUnit.test('initialize with strokeWidth with originX and originY', function(assert) {
+  QUnit.test('initialize with strokeWidth with originX and originY center/center', function(assert) {
     var done = assert.async();
     var path = new fabric.Path(
       'M 100 100 L 200 100 L 170 200 z',
-      { strokeWidth: 0, originX: 'center', originY: 'center' }
+      { strokeWidth: 4, originX: 'center', originY: 'center' }
     );
 
     assert.equal(path.left, 150);
     assert.equal(path.top, 150);
     done();
   });
 
+  QUnit.test('initialize with strokeWidth with originX and originY top/left', function(assert) {
+    var done = assert.async();
+    var path = new fabric.Path(
+      'M 100 100 L 200 100 L 170 200 z',
+      { strokeWidth: 4, originX: 'left', originY: 'top' }
+    );
+
+    assert.equal(path.left, 98);
+    assert.equal(path.top, 98);
+    done();
+  });
+
+    QUnit.test('initialize with strokeWidth with originX and originY bottom/right', function(assert) {
+    var done = assert.async();
+    var path = new fabric.Path(
+      'M 100 100 L 200 100 L 170 200 z',
+      { strokeWidth: 4, originX: 'right', originY: 'bottom' }
+    );
+
+    assert.equal(path.left, 202);
+    assert.equal(path.top, 202);
+    done();
+  });
+
   QUnit.test('set path after initialization', function (assert) {
     var done = assert.async();
     var path = new fabric.Path('M 100 100 L 200 100 L 170 200 z', REFERENCE_PATH_OBJECT);
@@ -137,6 +169,22 @@
     });
   });
 
+  QUnit.test('Path initialized with strokeWidth takes that in account for positioning', function (assert) {
+    var done = assert.async();
+    var path = new fabric.Path('M 100 100 L 200 100 L 170 200 z', REFERENCE_PATH_OBJECT);
+    updatePath(path, REFERENCE_PATH_OBJECT.path, true);
+    assert.deepEqual(path.toObject(), REFERENCE_PATH_OBJECT);
+    updatePath(path, REFERENCE_PATH_OBJECT.path, false);
+    var opts = { ...REFERENCE_PATH_OBJECT };
+    delete opts.path;
+    path.set(opts);
+    updatePath(path, 'M 100 100 L 300 100 L 200 300 z', true);
+    makePathObject(function (cleanPath) {
+      assert.deepEqual(path.toObject(), cleanPath.toObject());
+      done();
+    });
+  });
+
   QUnit.test('toString', function(assert) {
     var done = assert.async();
     makePathObject(function(path) {
@@ -172,7 +220,16 @@
     var done = assert.async();
     makePathObject(function(path) {
       assert.ok(typeof path.toSVG === 'function');
-      assert.equalSVG(path.toSVG(), '<g transform=\"matrix(1 0 0 1 200.5 200.5)\"  >\n<path style=\"stroke: rgb(0,0,255); stroke-width: 1; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n');
+      assert.equalSVG(path.toSVG(), '<g transform=\"matrix(1 0 0 1 200 200)\"  >\n<path style=\"stroke: rgb(0,0,255); stroke-width: 0; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n');
+      done();
+    });
+  });
+
+  QUnit.test('toSVG of path with a strokeWidth', function(assert) {
+    var done = assert.async();
+    makePathObject(function(path) {
+      path.strokeWidth = 2;
+      assert.equalSVG(path.toSVG(), '<g transform=\"matrix(1 0 0 1 201 201)\"  >\n<path style=\"stroke: rgb(0,0,255); stroke-width: 2; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n');
       done();
     });
   });
@@ -182,7 +239,7 @@
     makePathObject(function(path) {
       makePathObject(function(path2) {
         path.clipPath = path2;
-        assert.equalSVG(path.toSVG(), '<g transform=\"matrix(1 0 0 1 200.5 200.5)\" clip-path=\"url(#CLIPPATH_0)\"  >\n<clipPath id=\"CLIPPATH_0\" >\n\t<path transform=\"matrix(1 0 0 1 200.5 200.5) translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</clipPath>\n<path style=\"stroke: rgb(0,0,255); stroke-width: 1; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n', 'path clipPath toSVG should match');
+        assert.equalSVG(path.toSVG(), '<g transform=\"matrix(1 0 0 1 200 200)\" clip-path=\"url(#CLIPPATH_0)\"  >\n<clipPath id=\"CLIPPATH_0\" >\n\t<path transform=\"matrix(1 0 0 1 200 200) translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</clipPath>\n<path style=\"stroke: rgb(0,0,255); stroke-width: 0; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n', 'path clipPath toSVG should match');
         done();
       });
     });
@@ -195,7 +252,7 @@
       makePathObject(function(path2) {
         path.clipPath = path2;
         path.clipPath.absolutePositioned = true;
-        assert.equalSVG(path.toSVG(), '<g clip-path=\"url(#CLIPPATH_0)\"  >\n<g transform=\"matrix(1 0 0 1 200.5 200.5)\"  >\n<clipPath id=\"CLIPPATH_0\" >\n\t<path transform=\"matrix(1 0 0 1 200.5 200.5) translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</clipPath>\n<path style=\"stroke: rgb(0,0,255); stroke-width: 1; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n</g>\n', 'path clipPath toSVG absolute should match');
+        assert.equalSVG(path.toSVG(), '<g clip-path=\"url(#CLIPPATH_0)\"  >\n<g transform=\"matrix(1 0 0 1 200 200)\"  >\n<clipPath id=\"CLIPPATH_0\" >\n\t<path transform=\"matrix(1 0 0 1 200 200) translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</clipPath>\n<path style=\"stroke: rgb(0,0,255); stroke-width: 0; stroke-dasharray: none; stroke-linecap: butt; stroke-dashoffset: 0; stroke-linejoin: miter; stroke-miterlimit: 4; fill: rgb(255,0,0); fill-rule: nonzero; opacity: 1;\"  transform=\" translate(-200, -200)\" d=\"M 100 100 L 300 100 L 200 300 Z\" stroke-linecap=\"round\" />\n</g>\n</g>\n', 'path clipPath toSVG absolute should match');
         done();
       });
     });
@@ -272,7 +329,7 @@
     elPath.setAttributeNS(namespace, 'fill', 'red');
     elPath.setAttributeNS(namespace, 'opacity', '1');
     elPath.setAttributeNS(namespace, 'stroke', 'blue');
-    elPath.setAttributeNS(namespace, 'stroke-width', '1');
+    elPath.setAttributeNS(namespace, 'stroke-width', '0');
     elPath.setAttributeNS(namespace, 'stroke-dasharray', '5, 2');
     elPath.setAttributeNS(namespace, 'stroke-linecap', 'round');
     elPath.setAttributeNS(namespace, 'stroke-linejoin', 'bevel');