Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix not rendering multiple calls to html() #3271

Merged
merged 8 commits into from
Oct 7, 2021
Merged

Fix not rendering multiple calls to html() #3271

merged 8 commits into from
Oct 7, 2021

Conversation

KurtGokhan
Copy link
Contributor

Fixes #2899

@KurtGokhan KurtGokhan changed the title Multi html fix Fix not rendering multiple calls to html() Sep 24, 2021
@KurtGokhan
Copy link
Contributor Author

Hmm, this change generates a byte-wise different but visually same PDF for some tests.

image

I don't understand exactly why. But I guess we were previously keeping all the state changes html2canvas does and now that they are reverted, it is a completely different file byte-wise.

Should I regenerate reference files?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Sep 27, 2021

Yes, you would need to regenerate the reference PDFs.

@KurtGokhan
Copy link
Contributor Author

Updated reference files. As I said, there are no visual changes.

Copy link
Collaborator

@HackbrettXXX HackbrettXXX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. There seems to be differences in the html-margin-page-break-image file: on the first page the black outline is thinner than before. Same thing for some of the other files. Could you investigate why? Maybe that's actually the correct behavior. But then the lines should also be thin on the other pages.

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Oct 6, 2021

I hadn't noticed that.

At first glance, it looks like when a new page is created inside html, it is created with the state of that moment. So even after the state of first page is restored, the state of other pages remain the same. As you said the thin line is probably correct, but it should also be thin on other pages.

I don't know how to solve this yet but I will check when I find a chance.

Or I can just workaround the test by adding 3 pages before calling html so no new pages are added in html

@HackbrettXXX
Copy link
Collaborator

IMO, the pages that get created by context2d/html should have the regular initial state afterwards. If you find a quick solution we can fix it in this PR, but I'm also OK with fixing it in a separate PR. Then I think we should add the workaround to the tests, as you suggested.

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Oct 6, 2021

I somehow fixed the issue with graphical stack. But there was another bug closely related to it so my fix was in vain. The bug is that when every state is restored from the stack, the graphical state is loaded from Context2D's default values. But those defaults are not the same as document's defaults. So the current page gets overwritten with Context2D's default graphical state, while other pages retain the document's default state.

For example, you can see the default line width for document and Context2D

This needs a more comprehensive fix so for now I just did the test workaround.

There is also another issue. It is not exactly a bug but it is something counter-intuitive, and I believe it is the source of all the confusion. When you change the graphical state (for example by doing context2d.lineWidth = 2), it only writes that to the current page. If you change to another page, you will have a different lineWidth, even though the context2d.lineWidth is still 2. This is my opinion but I think every page should share the graphical state and that will make a lot of logic easier. This is quite a breaking change so also not included here.

@HackbrettXXX
Copy link
Collaborator

Ah, that sounds indeed like something that should be improved at the root of the problem. But not in this PR, of course ;)

To solve the issue, IMO, we should keep track of the current jsPDF state and compare it with the current context2d state each time we switch to another page. We should then apply only the difference to the current page. Otherwise we would serialize the whole context2d state each time we switch pages.

@HackbrettXXX HackbrettXXX merged commit 57120de into parallax:master Oct 7, 2021
@KurtGokhan KurtGokhan deleted the multi-html-fix branch October 7, 2021 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

only first html() call is rendered
3 participants