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 scaling issues in html module #3203

Merged
merged 5 commits into from
Sep 9, 2021
Merged

Fix scaling issues in html module #3203

merged 5 commits into from
Sep 9, 2021

Conversation

KurtGokhan
Copy link
Contributor

@KurtGokhan KurtGokhan commented Jul 8, 2021

I noticed that the elements did not respect the width option of html2canvas. This was causing scaling issues as seen in #3154, #3113, #2885. This can also be a solution to #2925

Let me try to describe the issue. Here are the relevant options:

  • windowWidth controls the width of the iframe, so that correct media queries are applied.
  • width controls the size of the output image.
  • scale scales the output image (does not change layout and element's proportions to each other etc.)

But none of these variables controls the size of the source element. The size of the source element is always its original size in the window. This causes issues because for example your window may be 2000px and your PDF is 1000px so it obviously won't fit. People use scale to compensate for that but it usually does not achieve a good result. I think the users almost always intend to make the source width and destination width equal. This PR does that.

If users want the old behavior after this change, they can still use the scale option to upscale the output image.

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Jul 12, 2021

@HackbrettXXX did you have any chance to check this?

I am having trouble with running tests on my local. What do you think about releasing this as a hotfix? I see many people are suffering from this issue.

Note: Updated the description to explain the issue better.

@HackbrettXXX
Copy link
Collaborator

I will look into this this week ;) I was really busy with other stuff the last weeks.

@KurtGokhan
Copy link
Contributor Author

Added tests. I had to update 2 pdf of old html tests. I think they were outdated. Also I am not sure if I should push dist folder or not.

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.

I like this solution. If I understand correctly, the width/height options are ignored by html2canvas anyway, because we pass an existing canvas to it and html2canvas will use the options only when creating a new canvas. So we can (and should) use these options to set the size of our canvas, which is basically our container element. We might need to multiply
this.opt.html2canvas.width with this.opt.html2canvas.scale, though. At least that's what html2canvas does when creating a new canvas:

https://github.com/niklasvh/html2canvas/blob/439e365ea8c703b528778a268dcfc3bf9ccad6a9/src/render/canvas/canvas-renderer.ts#L79-L84

However, I suggest we also introduce the width/height options on the top-level html options, because from a user's perspective that's where I would expect them to be. These options control the size of the canvas in jsPDF units.

In theory, the default for these options should be the page width and infinite height. However, this would be a breaking change, so we need to preserve the old default: Math.max(this.prop.src.clientWidth, this.prop.src.scrollWidth, this.prop.src.offsetWidth).

I also suggest we add another set of options: windowWidth/windowHeight. These should control the size of the imagined browser window in CSS pixels. The browser window size should then be scaled to the width/height options (which are given in jsPDF units). For this we can use the html2canvas scale option, if I'm correct. The default here should be width/height converted to CSS pixels.

What do we need to do to achieve that?

  1. Set containerCSS.width/height to windowWidth/windowHeight. If they're not provided, set them to width * internal.scaleFactor * "pt to px" (similar for height).
  2. Set html2canvas scale option to internal.scaleFactor * "pt to px" * width/windowWidth if not provided by the user already (maybe minimum of x and y scale?).

Please correct me if I'm wrong. Scaling math always messes with my brain ;)

The files in dist should not be committed. They are only updated on release. I will have a look at the tests once the CI works again (see #3208).

@KurtGokhan
Copy link
Contributor Author

KurtGokhan commented Jul 13, 2021

Ok, now I am confused as well. I have to write some tests with scale and see for myself. Otherwise it is hard to imagine what it is going to look like.

windowWidth/windowHeight already exists under html2canvas options and does what you said. Do you mean to add them to the top level html options?

You are right that this will be a breaking change if we reuse the same option.

Options under html2canvas are always pixel based as far as I know. As they are passed to html2canvas, I think it is better to keep it this way. Options under html2canvas are unrelated to the structure of PDF. That being said, the container is not related to html2canvas. So it makes sense to move this width option to top level. I wanted to achieve practicality instead of adding another option. But if we add another option to top level, it can be named containerWidth, elementWidth or just width.

If you ask me, I would make this a breaking change for the sake of practicality, fix all the confusion around width and scale, document it well, and create a major release. If you want to do this, I can help you as well.

To clarify something: You don't need to involve top level scale in html2canvas options. It just works without involving that. On top of that, html2canvas has another scale option which scales the image it outputs. That is less confusing than the top level scale option.

@HackbrettXXX
Copy link
Collaborator

HackbrettXXX commented Jul 14, 2021

windowWidth/windowHeight already exists under html2canvas options and does what you said. Do you mean to add them to the top level html options?

If I understand correctly, the html2canvas options only affect media queries and not the real window size. Because of that, we should add our own top level options that set the size of the container.

Options under html2canvas are always pixel based as far as I know. As they are passed to html2canvas, I think it is better to keep it this way. Options under html2canvas are unrelated to the structure of PDF. That being said, the container is not related to html2canvas. So it makes sense to move this width option to top level. I wanted to achieve practicality instead of adding another option. But if we add another option to top level, it can be named containerWidth, elementWidth or just width.

Yes, the html2canvas options are pixel based. But so far, jsPDF just interpreted pixels as if they were given in the current jsPDF unit. That's why html was basically only useful with the "pt" unit. I think width and windowWidth are good names. Of course we should document them well enough.

If you ask me, I would make this a breaking change for the sake of practicality, fix all the confusion around width and scale, document it well, and create a major release. If you want to do this, I can help you as well.

I'll think about that. I tend against a major release, because then we should also do everything in #3067, and I'll probably won't have time for that. I also think that a single default value for a new property doesn't really justify a major release.

To clarify something: You don't need to involve top level scale in html2canvas options. It just works without involving that. On top of that, html2canvas has another scale option which scales the image it outputs. That is less confusing than the top level scale option.

I'm not exactly sure what you mean by that. What do you mean by "another scale option"?

To clarify the use case I had in mind: As a user I want to be able to print my HTML page (let's say its 1000px X 2000px big) to a PDF page that's e.g. 210mm X 297mm. I want to be able to achieve that like this:

const doc = new jsPDF({ unit: "mm", format: [210, 297] })
doc.html(myMarkup, {
  width: 210,
  height: 297,
  windowWidth: 1000,
  windowHeight: 2000
})

Thanks btw for helping jsPDF. Your help is very much appreciated :)

@HackbrettXXX
Copy link
Collaborator

I've implemented it the way I had in mind. However, I only implemented width and windowWidth. I didn't implement height and windowHeight, because

  1. It would not be clear which scale to use if horizontal and vertical scale would differ
  2. Limiting the height is a really uncommon use case IMHO. Users usually want the entire HTML rendered. A browser would usually add scroll bars if the content overflows, but we can't do this in the PDF, so just render everything.

I also implemented it in a way that the options only have an effect, if both are specified. This way we won't have issues with weird default values or backwards compatibility.

@HackbrettXXX HackbrettXXX merged commit 18c56bf into parallax:master Sep 9, 2021
HackbrettXXX added a commit to jeffsieu/jsPDF that referenced this pull request Sep 9, 2021
 - update reference files from width/windowWidth PR (parallax#3203): there are no longer duplicate words
 - check in previously forgotten file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants