UPDATE: textWithLink method to cover multi-line annotated text

[Goku-kun]

This pull request addresses the issue #3253 and provides multi-line support for textWithLink() method.

[HackbrettXXX]

Thanks for the PR. I think we might be able to simplify it a little: there is a getTextDimensions function that returns width and height.

The tests seem to fail with this exception:

Invalid argument passed to jsPDF.scale in dist/jspdf.umd.min.js (line 86)

[Goku-kun]

The current implementation for fetching the height of a single line of link is much better than getting the height value from the getTextDimensions method.

Here's a comparison using the rect method.

Height using the current implementation.

It accounts for the full height including the ascenders and the descenders of the font.

Height using the getTextDimensions method

This doesn't account for the descenders or underscores. So, they're out of the area of the link.

I'm also attaching the code below to have an idea of the methods being used for calculating height.

// current implementation
var lineHeight = this.internal.getLineHeight() / this.internal.scaleFactor;

// getTextDimensions implementation
var lineHeight = this.getTextDimensions(text).h;

[Goku-kun]

Thanks for the PR. I think we might be able to simplify it a little: there is a getTextDimensions function that returns width and height.

The tests seem to fail with this exception:

Invalid argument passed to jsPDF.scale in dist/jspdf.umd.min.js (line 86)

I figured out the reason for the failure of this test. If options.maxWidth is not passed, then the totalHeight should be equal to lineHeight, which wasn't happening and so, totalHeight was getting converted to a NaN value in the link method.
With this new commit, that should be fixed.

[HackbrettXXX]

Ah, thanks for the clarification. Then I think the implementation is still flawed for some edge cases:

var numOfRows = Math.ceil(textWidth / maxWidth);

might give wrong results for longer texts, because each line might be a little shorter than maxWidth. With many lines, this difference adds up and we might get less lines than there actually are. You can use splitTextToSize to calculate the number of lines.

Please also add a test case.

[Goku-kun]

Yeah, I just saw that. The link breaks after 9 lines. The 10th line isn't supported unless Math.ceil(textWidth / maxWidth) + 1 is done to calculate the rows. I'll look into that and add a test case.

[Goku-kun]

Hey @HackbrettXXX.
Is there a guide to write the tests, or, should I just follow the structure in the other tests?

[HackbrettXXX]

See https://github.com/parallax/jsPDF/blob/master/CONTRIBUTING.md#building-and-testing-jspdf. Apart from that just have a look at the other tests ;)

[Goku-kun]

Hey, @HackbrettXXX

I don't think I need to write the test because there already exists one tests which checks for drawing the link using the textWithLink method.

// test/specs/annotations.spec.js line 58
it("should draw a link on the text with link after add page", () => {
    const doc = new jsPDF({
      unit: "px",
      format: [200, 300],
      floatPrecision: 2
    });

    doc.textWithLink("Click me!", 10, 10, {
      url: "https://parall.ax/"
    });

    doc.addPage("a4");

    doc.text("New page with difference size", 10, 10);

    comparePdf(doc.output(), "insertLinkAddPage.pdf", "annotations");
  });

I think that should address this issue and the previous test which was failing is also passing now.

I've also updated the implementation to find the total height using the splitToTextSize method as you suggested and it works perfectly. Kindly go through the files and let me know if there are any other changes to be made.

[HackbrettXXX]

I think, we should still add another test case with a multiline link. The code looks fine now.

[Goku-kun]

Hey @HackbrettXXX.

I think that should do it. I've added a test case under the annotations suite. Kindly review them and let me know if there's any another changes to be made.

[Goku-kun]

Okay? The test is passing in my system. I don't understand what's going wrong here. The failing test in the integration suite is the one that I wrote. And, it's successfully passing in my system.

[HackbrettXXX]

Thanks for adding the test case. Looks good. I think the tests fail because of floating point differences. You can make them green by passing {floatPrecision: 2} to the jsPDF constructor and updating the reference file.

[HackbrettXXX]

Alright, looks good now, I'll merge it.