refactor(Path, Polyline): BREAKING Svg import - cleanup #8857

asturur · 2023-04-23T17:11:18Z

Motivation

After fixing and investigating why SVG import was broken in #8879, a cleanup possibility was found.
We can remove the specific fromSVG import flag and the stroke offset correction from path parsing

Changes

creating Path/Polyon/Polyline with fromElement OUTSIDE the full svg parsing module will create different positioning, that is the same positioning that you would get using the constructor since now there is no more difference.
Is a breaking change, even if a good one in theory.

fromSVG one shot property is no more needed and is removed

github-actions · 2023-04-23T17:13:42Z

Build Stats

file / KB (diff)	bundled	minified
fabric	902.813 (-0.477)	304.635 (-0.183)

asturur · 2023-04-23T17:16:02Z

src/shapes/Polyline.ts

+        'left',
+        'top'
+      );
+    }


had to unwind setBoundingBox in order to apply the correct code or i should have had setBoundingBox returning a point like path does, but if i remember correctly that is not something we want to do.

We really not to sit down and look at open PRs
I believe I fixed this somewhere already

asturur · 2023-04-23T21:02:23Z

Master:

this branch

... can't npm link on the website

asturur · 2023-04-23T21:12:04Z

this branch:

Everything seems in place

asturur · 2023-04-27T22:54:51Z

@ShaMan123 i merged the other PR on which there wasn't much to say. But if you can look at this we can see how we see this fix and if we want to open a task for a better fix.
If you don't have time is totally fine i just thought i would give another shout at it.

ShaMan123 · 2023-04-27T23:01:59Z

Did you try #8438?

asturur · 2023-04-27T23:42:51Z

That seems to me only addressing the non nice code of a setDimension returning a point, that is tangential at the issue here that we are setting the point in the wrong position when originX and originY are different from top/left. It also doens't fix polygon.
So i don't think is related no.

ShaMan123

Let me check something

ShaMan123 · 2023-04-29T04:04:53Z

src/shapes/Path.ts

+    if (this.fromSVG) {
+      // if coming from SVG just assign top and left, those will be fixed
+      // later by remove removeTransformMatrixForSvgParsing using the weird
+      // _findCenterFromElement.
+      // This is not clear and is probably wrong, but as of now it works in conjuction
+      // with that other parsing function
+      this.set({ left: pathTL.x, top: pathTL.y });


I don't like this.
Can we instead handle the logic in fromElement and set the origin to left/top. Then prev logic should work

ShaMan123 · 2023-04-29T04:05:43Z

src/shapes/Polyline.ts

+      this.set({
+        left: calculatedLeft,
+        top: calculatedTop,
+      });


same
should be handled by fromElement
Especially since we want some day to extract all SVG logic to a standalone

ShaMan123 · 2023-04-29T04:06:52Z

src/shapes/Polyline.ts

+        'left',
+        'top'
+      );
+    }


We really not to sit down and look at open PRs
I believe I fixed this somewhere already

ShaMan123 · 2023-04-29T05:12:18Z

Did you try #8438?

It does fix this so it seems, the car test is flawless

ShaMan123 · 2023-04-29T05:15:42Z

Should should be closed in favor of #8438
Where is the gingerbread man svg?

ShaMan123 · 2023-04-29T05:16:53Z

I am not sure the test in #8438 covers what you have found , please double check with origin

ShaMan123 · 2023-04-29T05:24:07Z

I am thinking that originX/Y of a parsed path under a group should always be left/top regardless of anything

ShaMan123 · 2023-04-29T06:00:03Z

try this ts-path...test-path-svg-import-under-group

asturur · 2023-04-29T15:13:42Z

I am thinking that originX/Y of a parsed path under a group should always be left/top regardless of anything

No because that is a constrain that doesn't make sense. Is arbitrary. The only thing that should do is look as the SVG. You can't then work with objects that have different origins just because some come from SVG.

I will look and see if i can move the fix logic inside the fromElement, unless is too much extra code because i have basically redo what i do in constructor but in fromElement too.

The correct fix is probably in removeTransformMatrix code, but can't leave this broken too long.
I don't want to go back the easy way and revert a number of old PRs to fix this but not also get locked till i have time to look at a different solution.

asturur · 2023-04-29T15:15:25Z

try this ts-path...test-path-svg-import-under-group

Try this to do what? what should i try?

asturur · 2023-04-29T18:47:11Z

This half-way fix seems shaping better, but breaks gradients ( and probably patterns ).
If i can't fix gradients and patterns in a reasonabe amount of time i ll need to take the first fix.

asturur · 2023-04-29T21:38:17Z

Need to understand if the breakage of unit tests i have now are because tests are now wrong or if this is still wrong.
This is how i think the code should be.

svg parsing is independent of positioning becase it can find its own center from object svg data
positioning is center based instead of top,left so we can let go the strokeOffsetCorrection and so the fromSVG parameter

asturur · 2023-04-29T21:49:50Z

Yes, all the test failing have fromElement used outside the svg parser involved.
Before in order to use top and left as values, when the instance created from fromElement was created, the positioning was temporarly broken, waiting to be fixed from the svg parsing process.
Now is correct as soon as it is parsed, but i need to verify that visually for each case, so i will need some time.

asturur · 2023-04-29T22:13:08Z

Also Rect, Circle, Ellipse and Text are probably wrong when positioned from fromElement It doesn't really matter but if we can get them to work correctly better.

ShaMan123 · 2023-04-29T23:47:28Z

try this ts-path...test-path-svg-import-under-group

Try this to do what? what should i try?

test the svg import module

ShaMan123 · 2023-04-29T23:48:33Z

Need to understand if the breakage of unit tests i have now are because tests are now wrong or if this is still wrong. This is how i think the code should be.

svg parsing is independent of positioning becase it can find its own center from object svg data

positioning is center based instead of top,left so we can let go the strokeOffsetCorrection and so the fromSVG parameter

Sounds good. Take into consideration that stroke offset correction will die out with #8767

asturur · 2023-04-30T13:40:05Z

Ok at this point this is ready for me.
Maybe i will add a visual test that is more explanative than numbers.

github-actions · 2023-04-30T13:46:01Z

Coverage after merging test-svg-import into master will be

83.72%

Coverage Report

File	Stmts	Branches	Funcs	Lines	Uncovered Lines
index.node.ts	7.69%	100%	0%	14.29%	17, 20, 23, 29, 32, 35
src
ClassRegistry.ts	100%	100%	100%	100%
Collection.ts	94.71%	94.64%	86.67%	97.09%	101, 104, 207–208, 233–234
CommonMethods.ts	96.55%	87.50%	100%	100%	10
Intersection.ts	100%	100%	100%	100%
Observable.ts	87.23%	85.29%	84.62%	89.36%	144–145, 170–171, 39–40, 48, 57, 91, 99
Pattern.ts	92.21%	91.89%	90%	93.33%	114, 138, 149, 158, 51
Point.ts	100%	100%	100%	100%
Shadow.ts	98.36%	95.65%	100%	100%	178
cache.ts	97.06%	90%	100%	100%	57
config.ts	75%	66.67%	66.67%	82.76%	130, 138, 138, 138, 138, 138–140, 151–153
constants.ts	100%	100%	100%	100%
typedefs.ts	100%	100%	100%	100%
src/brushes
BaseBrush.ts	100%	100%	100%	100%
CircleBrush.ts	0%	0%	0%	0%	100, 102–104, 113, 113, 113, 115, 117, 119–121, 123–126, 133–134, 141, 143, 28–29, 37–41, 45–49, 56–59, 67–71, 73, 81, 81, 81, 81, 81–82, 84, 84, 84–87, 89, 97–98
PatternBrush.ts	97.06%	87.50%	100%	100%	21
PencilBrush.ts	91.01%	82.35%	100%	93.75%	122–123, 152, 152–154, 176, 176, 276, 280, 285–286, 68–69, 84–85
SprayBrush.ts	0%	0%	0%	0%	100–101, 103–104, 112, 112, 112, 112, 112–113, 115–116, 123–124, 126, 128–132, 141, 145–146, 146, 154, 154, 154–157, 159–162, 166–167, 169, 171–174, 177, 184–185, 187, 189–190, 192, 199–200, 202–203, 206, 206, 213, 213, 217, 22–23, 25–27, 27, 27–29, 33, 42, 49, 56, 63, 70, 89–91, 99
src/canvas
Canvas.ts	78.98%	77.54%	81.67%	79.60%	1000–1001, 1001, 1001–1003, 1005–1006, 1006, 1006, 1008, 1016, 1016, 1016–1018, 1018, 1018, 1024–1025, 1033–1034, 1034, 1034–1035, 1040, 1042, 1073–1075, 1078–1079, 1083–1084, 1197–1199, 1202–1203, 1276, 1395, 1518, 1588, 162, 187, 296–297, 300–304, 309, 332–333, 338–343, 363, 363, 363–364, 364, 364–365, 37, 373, 378–379, 379, 379–380, 382, 391, 397–398, 398, 398, 41, 441, 449, 453, 453, 453–454, 456, 538–539, 539, 539–540, 546, 546, 546–548, 568, 570, 570, 570–571, 571, 571, 574, 574, 574–575, 578, 587–588, 590–591, 593, 593–594, 596–597, 609–610, 610, 610–611, 613–617, 623, 630, 670, 670, 670, 672, 674–678, 684, 690, 690, 690–691, 693, 696, 701, 714, 741, 741, 799–800, 800, 800–801, 803, 806–807, 807, 807–808, 810–811, 814, 814–816, 819–820, 890, 902, 909, 930, 962, 983–984
SelectableCanvas.ts	94.39%	91.16%	94.55%	96.63%	1112, 1112–1113, 1116, 1136, 1136, 1194, 1247–1248, 1269, 1277, 1402, 1404, 1406–1407, 511, 691–692, 694–695, 695, 695, 744–745, 806–807, 860–862, 894, 899–900, 927–928
StaticCanvas.ts	96.01%	91.48%	100%	97.93%	1128–1129, 1129, 1129–1130, 1250, 1260, 1314–1315, 1318, 1353–1354, 1431, 1440, 1440, 1444, 1444, 1491–1492, 1706–1707, 327–328, 345, 425–426, 428–429, 790, 802–803, 888
TextEditingManager.ts	100%	100%	100%	100%
src/color
Color.ts	92.16%	86.49%	100%	94.29%	330–331, 335–336, 339–340, 58, 88–89, 89, 91, 91, 91–92, 94–95
color_map.ts	100%	100%	100%	100%
constants.ts	100%	100%	100%	100%
util.ts	100%	100%	100%	100%
src/controls
Control.ts	93.90%	88.89%	90.91%	97.73%	235, 322, 322, 357
changeWidth.ts	100%	100%	100%	100%
commonControls.ts	100%	100%	100%	100%
controlRendering.ts	81.63%	78%	100%	84.78%	106, 111, 121, 121, 45, 50, 61, 61, 65–72, 81–82
drag.ts	100%	100%	100%	100%
polyControl.ts	5.97%	0%	0%	11.11%	100, 105, 119, 119, 119, 119, 119, 121–124, 124, 127, 134, 17, 25–29, 29, 29, 29, 29, 29, 29, 29, 50–56, 56, 56, 56, 56, 58, 63–64, 66, 76, 82–83, 83, 83–84, 88–90, 90, 90, 90, 90, 92
rotate.ts	19.57%	12.50%	50%	21.43%	41, 45, 51, 51, 51–52, 55–57, 59, 59, 59, 59, 59–61, 61, 61–63, 65, 65, 65–67, 67, 67–68, 73, 73, 73–74, 76, 78, 80–81
scale.ts	93.57%	92.94%	100%	93.67%	129–130, 132–134, 148–149, 181–183, 42
scaleSkew.ts	78.79%	64.29%	100%	85.71%	27, 29, 29, 29, 31, 33, 35
skew.ts	91.03%	79.31%	100%	97.67%	130–131, 162–163, 170, 176, 178
util.ts	100%	100%	100%	100%
wrapWithFireEvent.ts	100%	100%	100%	100%
wrapWithFixedAnchor.ts	100%	100%	100%	100%
src/env
browser.ts	84.21%	77.78%	50%	100%	14, 17
index.ts	100%	100%	100%	100%
node.ts	76.92%	50%	66.67%	88.24%	27, 31–32, 32, 32
src/filters
BaseFilter.ts	21.93%	22.81%	32%	19.05%	106–109, 109, 109–110, 116, 116, 116–119, 137, 155, 169–174, 178–179, 179, 179–182, 182, 182,

asturur · 2023-04-30T20:47:50Z

Added small test that position polygon/path on a grid

asturur · 2023-04-30T20:56:55Z

@ShaMan123 in the test there is a green line, that is there to represent how broken is the line currently, and that probably will be fixed with the line clean up.

asturur · 2023-04-30T20:57:09Z

I won't do anything anymore here unless there are comment to address

asturur · 2023-05-06T21:50:15Z

Those changes overlap with the SVG parse fromElement from callback to promise.
Those changes the position of the parsed path, the other the fact that the parsing is async and addressing tests will be a mess

asturur · 2023-05-06T23:15:18Z

Before line improvements:

After line improvements:

ShaMan123 · 2023-05-06T23:47:32Z

Well done!

ShaMan123

looks good

ShaMan123 · 2023-05-07T06:07:39Z

test/unit/polygon.js

+
+    var polygon = new fabric.Polygon(getPoints(), { strokeWidth: 2, originX: 'left', originY: 'top' });
+
+    assert.equal(polygon.left, 9);


Ohh snapshot testing, I wish you were here!!

asturur added 2 commits April 23, 2023 18:32

fix import bug

a2b3823

ok previous fix was wrong

493ee8a

asturur commented Apr 23, 2023

View reviewed changes

asturur changed the title ~~fix(Path, Polyline): Svg import fix~~ fix(Path, Polyline): Issue 8856 Svg import fix Apr 23, 2023

ShaMan123 reviewed Apr 29, 2023

View reviewed changes

halfway there

af688c5

asturur added 4 commits April 29, 2023 21:56

fix gradients

18efa17

some es6 code

785321d

Merge branch 'master' into test-svg-import

81611a9

need to think about this

bc4fbae

invalid comment

f3d595e

asturur changed the title ~~fix(Path, Polyline): Issue 8856 Svg import fix~~ fix(Path, Polyline): Svg import - cleanup Apr 30, 2023

asturur added 7 commits April 30, 2023 13:01

Merge branch 'master' into test-svg-import

65170cd

Merge branch 'master' into test-svg-import

83a1dd0

added changelog

4d805b0

Path test passing

49b4ae9

added more tests to cover differences

7e09ec1

polyline tests ok

a4d8eff

polygon tests

2582b13

asturur changed the title ~~fix(Path, Polyline): Svg import - cleanup~~ fix(Path, Polyline): BREAKING Svg import - cleanup Apr 30, 2023

added a test

0f09486

stronger test

c5d51c9

asturur requested a review from ShaMan123 April 30, 2023 20:57

asturur added 2 commits May 5, 2023 22:46

Merge branch 'master' into test-svg-import

158394b

Merge branch 'master' into test-svg-import

b5498d2

asturur added 3 commits May 7, 2023 01:03

Merge branch 'master' into test-svg-import

a3c4faa

correct changelog

c3b7054

improved line test

c18a6db

asturur changed the title ~~fix(Path, Polyline): BREAKING Svg import - cleanup~~ refactor(Path, Polyline): BREAKING Svg import - cleanup May 6, 2023

ShaMan123 approved these changes May 7, 2023

View reviewed changes

asturur merged commit d4eb484 into master May 7, 2023
18 checks passed

asturur deleted the test-svg-import branch May 7, 2023 14:56

asturur mentioned this pull request May 19, 2023

Version 6.0.0-beta6 #8935

Merged


		var polygon = new fabric.Polygon(getPoints(), { strokeWidth: 2, originX: 'left', originY: 'top' });

		assert.equal(polygon.left, 9);

refactor(Path, Polyline): BREAKING Svg import - cleanup #8857

refactor(Path, Polyline): BREAKING Svg import - cleanup #8857

Conversation

asturur commented Apr 23, 2023 • edited

Motivation

Changes

github-actions bot commented Apr 23, 2023 • edited

asturur Apr 23, 2023

Choose a reason for hiding this comment

ShaMan123 Apr 29, 2023

Choose a reason for hiding this comment

asturur commented Apr 23, 2023

asturur commented Apr 23, 2023

asturur commented Apr 27, 2023

ShaMan123 commented Apr 27, 2023

asturur commented Apr 27, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 Apr 29, 2023

Choose a reason for hiding this comment

ShaMan123 Apr 29, 2023

Choose a reason for hiding this comment

ShaMan123 Apr 29, 2023

Choose a reason for hiding this comment

ShaMan123 commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023

asturur commented Apr 29, 2023 • edited

asturur commented Apr 29, 2023

asturur commented Apr 29, 2023

asturur commented Apr 29, 2023

asturur commented Apr 29, 2023

asturur commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023

ShaMan123 commented Apr 29, 2023 • edited

asturur commented Apr 30, 2023 • edited

github-actions bot commented Apr 30, 2023 • edited

asturur commented Apr 30, 2023

asturur commented Apr 30, 2023

asturur commented Apr 30, 2023

asturur commented May 6, 2023

asturur commented May 6, 2023

ShaMan123 commented May 6, 2023

ShaMan123 left a comment

Choose a reason for hiding this comment

ShaMan123 May 7, 2023

Choose a reason for hiding this comment

asturur commented Apr 23, 2023 •

edited

github-actions bot commented Apr 23, 2023 •

edited

asturur commented Apr 29, 2023 •

edited

ShaMan123 commented Apr 29, 2023 •

edited

asturur commented Apr 30, 2023 •

edited

github-actions bot commented Apr 30, 2023 •

edited