parallax / jsPDF

Client-side JavaScript PDF generation for everyone.
https://parall.ax/products/jspdf
MIT License
29.16k stars 4.65k forks source link

doc.text() line breaks issue when combined with maxWidth parameter #2995

Closed esjs closed 3 years ago

esjs commented 3 years ago

I've been struggling with adding line breaks into text, and when checked source code noticed that what you do is first split text into separate lines https://github.com/MrRio/jsPDF/blob/c44b9c1e02fd83e25a226f4e2ff55d20eee83743/src/jspdf.js#L3570-L3576 and then add them again and calculate line breaks based on maxWidth if present https://github.com/MrRio/jsPDF/blob/c44b9c1e02fd83e25a226f4e2ff55d20eee83743/src/jspdf.js#L3602-L3612

Here is a codesandbox example which shows the problem (use preview in new window to make PDF generation work) https://codesandbox.io/s/jspdf-line-break-max-width-issue-pg629

I've tried to move code which splits by line breaks below code for maxWidth calculations and seems to be working ok. Are there any issues with this approach, or can I prepare merge request and it would be approved?

Thank you, Vladimir

HackbrettXXX commented 3 years ago

What exactly is the issue? Could you provide a small sample together with it's current output and the output you would expect? Thanks.

esjs commented 3 years ago

What exactly is the issue? Could you provide a small sample together with it's current output and the output you would expect? Thanks.

I've attached a codesandbox example which demonstrates that if you insert line-break sequence and set maxWidth properties, those line-breaks will be ignored. With proposed fix, everything works as expected. See attach before/after .pdf files

print_after.pdf test_before.pdf

HackbrettXXX commented 3 years ago

Ah, yes this issue has been reported multiple times already. However, fixing this could be seen as a breaking behavior change, which is why we should be careful. Maybe we can implement the fix behind a hot-fix flag? See here.

esjs commented 3 years ago

Ah, yes this issue has been reported multiple times already. However, fixing this could be seen as a breaking behavior change, which is why we should be careful. Maybe we can implement the fix behind a hot-fix flag? See here.

Sounds like a very good idea, that's why I ask about breaking changes in first message. Do you think name "text_newline_max_width" will be descriptive enough? If so, I can update merge request with this parameter in mind. Also, are there any special files that should be updated besides HOTFIX_README.md to reflect this change?

HackbrettXXX commented 3 years ago

I think the hotfix name is good :) I think we should document this hotfix in the HOTFIX_README as well as in the JSDoc of the text method.

esjs commented 3 years ago

Hi @HackbrettXXX, I've been working on tests for hotfix, and couldn't make them work, but then I realized that version of jspdf.js that we have in src is different from what we have in dist/jspdf.es.js. And in version inside src everything already works as expected.

See src jspdf https://github.com/MrRio/jsPDF/blob/master/src/jspdf.js#L3602-L3612

and inside dist https://github.com/MrRio/jsPDF/blob/master/dist/jspdf.es.js#L4038-L4046

Here is a fix for this issue https://github.com/esjs/jsPDF/commit/4ccec9bd84b2fa16bd476010f8d341ad5091eb1d

Any ideas on how this could've happened?

HackbrettXXX commented 3 years ago

The files in the dist folder are only updated on release. So if the issue is already fixed, I guess we can simply close this issue and the PR. I will prepare a new release in the next 2-3 weeks.

esjs commented 3 years ago

Got it, will wait for release, thanks!