search

yWorks / svg2pdf.js

A javascript-only SVG to PDF conversion utility that runs in the browser. Brought to you by yWorks - the diagramming experts
MIT License
654 stars 101 forks source link

Incorrect text-svg scaling #95

Closed Fadiabb closed 5 years ago

Fadiabb commented 5 years ago

Describe the bug Rendering of text-svg element on pdf does not have the right scaling svg2pdf.js version: 38f10d329807d2a671e78695a4504dc3ebbb7a33

To Reproduce using mm unit with [420,297] (a3) format in jspdf constructor and scale= 1 

<script type="text/javascript">
      function downloadpdf() {
        const pdf = new jsPDF("l", "mm", [420, 297]);
        const SVGAElement = document.getElementById("svg");
        svg2pdf(SVGAElement, pdf, {
          xOffset: 10,
          yOffset: 60,
          scale: 1
        });
        pdf.save("a.pdf");
      }
 </script>
<svg id="svg" width="100%" height="100%">
        <g class="main" transform="translate(160, 20)">
          <path
            class="domain"
            stroke="black"
            d="M0.5,238.5V0.5"
            fill="none"
          ></path>
          <text
            text-anchor="middle"
            transform="translate(-45, 119) rotate(-90)"
          >
            Some Text to test !!!!
          </text>
          <g fill="none" font-size="10" text-anchor="start">
            <g opacity="1" transform="translate(0,153.32105263157894)">
              <text fill="black" x="-25">3.50</text>
            </g>
            <g opacity="1" transform="translate(0,130.54593301435406)">
              <text fill="black" x="-25">4.00</text>
            </g>
            <g opacity="1" transform="translate(0,107.77081339712919)">
              <text fill="black" x="-25">4.50</text>
            </g>
            <g opacity="1" transform="translate(0,84.99569377990431)">
              <text fill="black" x="-25">5.00</text>
            </g>
          </g>
        </g>
      </svg>

The scale and the position of the rendered elements on pdf are not right . Screenshots the original svg element on html page: scaling-issue

the generate pdf: scaling-issue-pdf

yGuy commented 5 years ago

Could be that this is just a matter of font-size not being inherited from the g element?

HackbrettXXX commented 5 years ago

Font sizes are always given in pt no matter what unit you specify in the jsPDF constructor. This is somewhat unintuitive but due to backwards compatibility and compatibility with the original jsPDF. This means you basically can use svg2pdf only if you set the unit of jsPDF to pt.

bernhardreiter commented 5 years ago

As far as I remember jsPDF can be used with mm and fonts. And the browser interprets this differently.

HackbrettXXX commented 5 years ago

Of course you can use jsPDF with mm and fonts. But you still set the font size in pt and all other lengths in mm. And unfortunately we have no chance figuring out the current scale/unit of jsPDF as this is not public API.

A workaround would be to set the unit of jsPDF to pt (and give the page size accordingly) and set the scale of svg2pdf to 25.4 / 72 (hope I got the fraction the right way round :D).

bernhardreiter commented 5 years ago

In the above example the scale/unit seems to be accessible with

console.log(pdf.internal.scaleFactor);

The source code comments hint towards that API.internal is meant to be used by plugins, so using it from svg2pdf seems feasible. https://github.com/yWorks/jsPDF/blob/3561de8fa953a48e98a08255bb97c9d8a9028eff/jspdf.js#L1868

HackbrettXXX commented 5 years ago

Yeah, you are right. So we could convert the fontsize accordingly.

bernhardreiter commented 5 years ago

The following patch already seems to be an improvement, but the positioning of the rotated text is still not correct, so something is still wrong:

diff --git a/src/svg2pdf.js b/src/svg2pdf.js
index cd7b82b..383a245 100644
--- a/src/svg2pdf.js
+++ b/src/svg2pdf.js
@@ -295,7 +295,7 @@ SOFTWARE.
     this.fillOpacity = 1.0;
     // this.fillRule = null;
     this.fontFamily = null;
-    this.fontSize = 16;
+    this.fontSize = 16 * _pdf.internal.scaleFactor;
     this.fontStyle = null;
     // this.fontVariant = null;
     this.fontWeight = null;
@@ -321,7 +321,7 @@ SOFTWARE.
     attributeState.fillOpacity = 1.0;
     // attributeState.fillRule = "nonzero";
     attributeState.fontFamily = "times";
-    attributeState.fontSize = 16;
+    attributeState.fontSize = 16 * _pdf.internal.scaleFactor;
     attributeState.fontStyle = "normal";
     // attributeState.fontVariant = "normal";
     attributeState.fontWeight = "normal";
@@ -1553,7 +1553,7 @@ SOFTWARE.

     var dx, dy, xOffset = 0;

-    var pdfFontSize = _pdf.getFontSize();
+    var pdfFontSize = _pdf.getFontSize() / _pdf.internal.scaleFactor;
     var textX = toPixels(node.getAttribute('x'), pdfFontSize);
     var textY = toPixels(node.getAttribute('y'), pdfFontSize);

@@ -1800,7 +1800,7 @@ SOFTWARE.

     var fontSize = getAttribute(node, "font-size");
     if (fontSize) {
-      attributeState.fontSize = parseFloat(fontSize);
+      attributeState.fontSize = parseFloat(fontSize*_pdf.internal.scaleFactor);
     }

     var textAnchor = getAttribute(node, "text-anchor");

Also I'm not sure about reversing the factor on pdfFontSize.

bernhardreiter commented 5 years ago

With the additional following change, the svg from this test seems to render fine:

--- a/src/svg2pdf.js
+++ b/src/svg2pdf.js
@@ -1377,7 +1379,7 @@ SOFTWARE.
     var fontFamily = attributeState.fontFamily;
     var measure = getMeasureFunction(fontFamily);

-    return measure(text, attributeState.fontFamily, attributeState.fontSize + "px", attributeState.fontStyle, attributeState.fontWeight);
+    return measure(text, attributeState.fontFamily, attributeState.fontSize / _pdf.internal.scaleFactor + "px", attributeState.fontStyle, attributeState.fontWeight);
   }

   /**
bernhardreiter commented 5 years ago

To change this into a nice improvement, it has to be decided if the attributeState.fontSize shall be in page units or in points. The above patch-set assumes that it shall be page units. Then comments and documentation will have to be added.

yGuy commented 5 years ago 
     if (fontSize) {
-      attributeState.fontSize = parseFloat(fontSize);
+      attributeState.fontSize = parseFloat(fontSize*_pdf.internal.scaleFactor);
     }

This doesn't look right to me. Shouldn't this better be:

     if (fontSize) {
-      attributeState.fontSize = parseFloat(fontSize);
+      attributeState.fontSize = parseFloat(fontSize) *_pdf.internal.scaleFactor;
     }
bernhardreiter commented 5 years ago

@yGuy good catch. It works because of the implicit type conversion to number.

When testing I've found that there is an additional defect when using em as font size. I've change one of the axis labels to read:

<g opacity="1" font-size="2em" transform="translate(0,84.99569377990431)">
              <text fill="black" x="-25">5.00</text>

Shows fine on the browser, but not on svg2pdf (current master) with or without the patch. (For today I'll probably stop working on this, just thought you wanted to know.)

HackbrettXXX commented 5 years ago

Yeah, units are not really supported (em only for the dx/dy attributes).

I think having unscaled values on AttributeState and only applying the scale when setting the font size is better, since this way we stay mostly independent of jsPDF's strange behavior here.

bernhardreiter commented 5 years ago

About "em" for fonts: Where shall I report this best? Just a new issue for svg2pdf?

About where to scale the fonts correctly in the code: Setting the fonts in "pt", even if the positioning is done in millimeter looks okay for me, as this is the unit people are traditionally using for fonts. So I'm not sure where you would like to scale it in the code. For coordination: Are you preparing the chance or shall I do it?

HackbrettXXX commented 5 years ago

About "em" for fonts: Where shall I report this best? Just a new issue for svg2pdf?

A new issue, yes.

About where to scale the fonts correctly in the code: Setting the fonts in "pt", even if the positioning is done in millimeter looks okay for me, as this is the unit people are traditionally using for fonts. So I'm not sure where you would like to scale it in the code.

Yeah, I still think we should only scale it when calling jsPDF methods since this is kind of a workaround for jsPDF's behavior. I also think this makes the diff much smaller.

For coordination: Are you preparing the chance or shall I do it?

I'm quite busy now so I would be glad if you could do that.

bernhardreiter commented 5 years ago

Created #100

bernhardreiter commented 5 years ago

The behavour of jsPDF maybe problematic here, so it would be cool take this up with jsPDF upstream on days, e.g. https://github.com/MrRio/jsPDF/issues/2294 described a few scaling issues with fonts. The conversation in the issue reads like joining jsPDF and jsPDF-yworks is a worthwhile goal.