loadSVGFromString issue with letter-spacing

[rikkarv]

Version
2.3.0 & 2.4.4

Test Case
https://jsfiddle.net/rikkarv/98mc60ow/8/ (for version 2.3.0)
https://jsfiddle.net/rikkarv/98mc60ow/7/ (for version 2.4.4)

Information about environment
chrome

Steps to reproduce
click on svg's (outline with dashed blue) and see output in fabric (outline with dashed red)

Expected Behavior
svg text should be imported to canvas with correct letter-spacing, despite the letter-spacing unit

Actual Behavior
If (fabric version 2.3.0) {
svg using letter-spacing value as "0.5em" works perfectly and outputs correct object.charSpacing value (500);
svg using letter-spacing value as "10" doesn't work and outputs a wrong object.charSpacing value (625);
} else if (fabric version 2.3.0) {
svg using letter-spacing value as "0.5em" doesn't work and outputs a wrong object.charSpacing value (0.5em);
svg using letter-spacing value as "10" doesn't work and outputs a wrong object.charSpacing value (10);
}

[rikkarv]

In my tests i get the following:

- Since version 2.4.1 to 2.4.4:

• using svg with unit 'em', the method fabric.normalizeValue() calls parseUnit(0.5rem, 20px) and the parseUnit() returns NaN because fontSize is a string (20px instead of 20);

• using svg with unit 'px', the method fabric.normalizeValue() calls parseUnit(10, 20px) and the parseUnit() returns 10, however parsed is NaN because fontSize is a string (20px instead of 20);

In both cases we get NaN because we are doing maths with a variable fontSize that is a string.
And in the final line of fabric.normalizeValue() we return the original value (0.5em in the first case and 10 in the second case) because isNaN(parsed) ? value : parsed

For this version i suggest changing to:
else if (attr === 'charSpacing') {
fixFontSize = Number(fontSize.replace('px', '');
parsed = parseUnit(value, fixFontSize) / fixFontSize * 1000;
}

- Since version 2.3.0 to 2.4.0:

• using svg with unit 'em', works very good, the parsed value is 500 (= 0.5em);

• using svg with unit 'px', the method fabric.normalizeValue() calls parseUnit(-3, 16) and the parseUnit() returns 10, the parsed value is 625 because fontSize is default value (16) and not the real value (20);

[rikkarv]

Please note that i pointed Chrome as environment.
The test cases in fiddle are easier to understand using Chrome, IE/Edge does not support letter-spacing without 'px' and that it is the reason why the second SVG is not right if using IE/Edge.
Using Chrome should be visible that both svg's have the same letter-spacing.
However, despite the lack of support from IE/Edge, in my case, it is mandatory to support "letter-spacing: 10" without refering "px". I think that Adobe Illustrator outputs svg like that.

[asturur]

The px should be solved.
I'll take the reference of https://jsfiddle.net/rikkarv/98mc60ow/7/ (for version 2.4.4) and be sure is fixed.
I did not get exactly the comment about fontsie being 16 vs 20. Can you clarify more?

[rikkarv]

Now i'm not sure because i don't have access to my computer but I think that by doing console.log(fontSize) in normalizeValue() I Was getting 16px, which is fabric default in cases where fontSize is null, instead of 20px which is the value set in the svg. But maybe it happen in other case that i tried. I will confirm that. Anyway, this situation doesn't happen in version 2.4.4. However, in version 2.4.4 there is also a issue where using 'em' and the problem as to do with fontSize variable being a string instead of number. No dia 08/12/2018, às 14:43, Andrea Bogazzi <notifications@github.com<mailto:notifications@github.com>> escreveu: The px should be solved. I'll take the reference of https://jsfiddle.net/rikkarv/98mc60ow/7/ (for version 2.4.4) and be sure is fixed. I did not get exactly the comment about fontsie being 16 vs 20. Can you clarify more? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#5417 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AmlY_osLGsMG2YEOOm9jMxFupZeip3Zeks5u29AEgaJpZM4Y-9fe>.

[rikkarv]

I'm looking at changelog, and in version 2.4.1 there is commit 0a9466a that come from "fix letterspace parsing #5258" and i believe from issue #5192.
Before 2.4.1 element.getAttribute('font-size') was returning null then fabric was always setting fontSize to fabric.Text.DEFAULT_SVG_FONT_SIZE (that returned the number 16). With the commit 0a9466a element.getAttribute('font-size') was replaced by ownAttributes['font-size'] that returns a string and breaks all the maths done at parseUnit() called by normalizeValue()

[asturur]

that commit did fix svgs anyway, so probably the truth is in the middle.

[asturur]

this seems more correct to me.
I ll link you to the code diffs, let me know what you think of it.

[rikkarv]

Thank you Andrea.

Later or tomorrow i will see in detail your idea.

However, i think the version before 2.4.1, was better. It seems that in those older versions the problem was not getting font-size when set inline in style attribute instead of font-size own attribute (like: style="font-size: 16px;").

Anyway, tomorrow i will you give notice of my tests in your pull request.

Please note that in your pull request "fontSize = parseUnit(ownAttributes['font-size'], parentFontSize);" ignores parentFontSize if ownAttributes['font-size'] has 'px' unit (because it falls in default case)

[asturur]

If it has px it has to ignore the parent.
The parent is used only when you are parsing a font-size expressed in 'em', so you need to read the container font-size.
While for other things like strokeWidth or letterSpacing expressed in 'em' you refer to current font-size.

I ll merge the code without publishing 2.4.5, let me know if you it still do not work.

[rikkarv]

The picture above is some kind of output of the 'test/visual/assets/svg_text_letterspacing.svg' file?

If so, you have to notice that the first text tag has font-size: 20px and letter-spacing: 20px which is equivalent to letter-spacing: 1em, right? So the letter-spacing of the first tag should be the double of the space compared to the second tag.

In the third and fourth tag, the 'em' is relative to 16px default?

Can you get the objects charSpacing property?

[asturur]

well the visual test compares what fabric parse and render with what the browser rendered on the same svg, important is that match.

[rikkarv]

I have already tested your changes and

ownAttributes['font-size'] = fontSize = parseUnit(ownAttributes['font-size'], parentFontSize);

is really doing his job. Now i have fontSize as a number instead of a string and the following maths doesn't break.

Thank you very much for your effort

[asturur]

so we are ready for 2.4.5 Il giorno lun 10 dic 2018 alle ore 14:20 Ricardo Carvalho < notifications@github.com> ha scritto:

…

I have already tested your changes and ownAttributes['font-size'] = fontSize = parseUnit(ownAttributes['font-size'], parentFontSize); is really doing his job. Now i have fontSize as a number instead of a string and the following maths doesn't break. Thank you very much for your effort — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#5417 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABI4QJ_Y_34iKqcRWSjFwnDBFppFT42rks5u3l9_gaJpZM4Y-9fe> .

[rikkarv]

test.zip