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 margin values not working for html #2977

Merged
merged 23 commits into from
Sep 14, 2021
Merged
Show file tree
Hide file tree
Changes from 17 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 67 additions & 22 deletions src/modules/context2d.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ import { console } from "../libs/console.js";
this.currentPoint = ctx.currentPoint || new Point();
this.miterLimit = ctx.miterLimit || 10.0;
this.lastPoint = ctx.lastPoint || new Point();
this.margin = ctx.margin || [0, 0, 0, 0];
this.prevPageLastElemOffset = ctx.prevPageLastElemOffset || 0;

this.ignoreClearRect =
typeof ctx.ignoreClearRect === "boolean" ? ctx.ignoreClearRect : true;
Expand Down Expand Up @@ -157,6 +159,20 @@ import { console } from "../libs/console.js";
}
});

/**
* @name margin
* @type {array}
* @default [0, 0, 0, 0]
*/
Object.defineProperty(this, "margin", {
get: function() {
return _ctx.margin;
},
set: function(value) {
_ctx.margin = value;
}
});

var _autoPaging = false;
/**
* @name autoPaging
Expand Down Expand Up @@ -1457,29 +1473,35 @@ import { console } from "../libs/console.js";
for (var i = min; i < max + 1; i++) {
this.pdf.setPage(i);

var topMargin = (i === min ? this.posY + this.margin[0] : this.margin[0]);
var firstPageHeight = this.pdf.internal.pageSize.height - this.posY - this.margin[0] - this.margin[2];
var pageHeightMinusMargin = this.pdf.internal.pageSize.height - this.margin[0] - this.margin[2];
var pageWidthMinusMargin = this.pdf.internal.pageSize.width - this.margin[1];
var previousPageHeightSum = i === 1 ? 0 : firstPageHeight + (i - 2) * pageHeightMinusMargin;

if (this.ctx.clip_path.length !== 0) {
var tmpPaths = this.path;
clipPath = JSON.parse(JSON.stringify(this.ctx.clip_path));
this.path = pathPositionRedo(
clipPath,
this.posX,
-1 * this.pdf.internal.pageSize.height * (i - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin + this.ctx.prevPageLastElemOffset
);
drawPaths.call(this, "fill", true);
this.path = tmpPaths;
}
var tmpRect = JSON.parse(JSON.stringify(xRect));
tmpRect = pathPositionRedo(
[tmpRect],
this.posX,
-1 * this.pdf.internal.pageSize.height * (i - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin + this.ctx.prevPageLastElemOffset
)[0];
this.pdf.addImage(
img,
"JPEG",
tmpRect.x,
tmpRect.y,
tmpRect.w,
Math.min(tmpRect.w, this.pdf.internal.pageSize.width - this.margin[1] - tmpRect.x),
tmpRect.h,
null,
null,
Expand All @@ -1504,7 +1526,7 @@ import { console } from "../libs/console.js";
var getPagesByPath = function(path, pageWrapX, pageWrapY) {
var result = [];
pageWrapX = pageWrapX || this.pdf.internal.pageSize.width;
pageWrapY = pageWrapY || this.pdf.internal.pageSize.height;
pageWrapY = pageWrapY || this.pdf.internal.pageSize.height - this.margin[0] - this.margin[2];

switch (path.type) {
default:
Expand Down Expand Up @@ -1653,22 +1675,27 @@ import { console } from "../libs/console.js";
this.lineWidth = lineWidth;
this.lineJoin = lineJoin;

var topMargin = (k === min ? this.posY + this.margin[0] : this.margin[0]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be 1 instead of min here as well.

Suggested change
var topMargin = (k === min ? this.posY + this.margin[0] : this.margin[0]);
var topMargin = (k === 1 ? this.posY + this.margin[0] : this.margin[0]);

var firstPageHeight = this.pdf.internal.pageSize.height - this.posY - this.margin[0] - this.margin[2];
var pageHeightMinusMargin = this.pdf.internal.pageSize.height - this.margin[0] - this.margin[2];
var previousPageHeightSum = k === 1 ? 0 : firstPageHeight + (k - 2) * pageHeightMinusMargin;

if (this.ctx.clip_path.length !== 0) {
var tmpPaths = this.path;
clipPath = JSON.parse(JSON.stringify(this.ctx.clip_path));
this.path = pathPositionRedo(
clipPath,
this.posX,
-1 * this.pdf.internal.pageSize.height * (k - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin + this.ctx.prevPageLastElemOffset
);
drawPaths.call(this, rule, true);
this.path = tmpPaths;
}
tmpPath = JSON.parse(JSON.stringify(origPath));
this.path = pathPositionRedo(
tmpPath,
this.posX,
-1 * this.pdf.internal.pageSize.height * (k - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin + this.ctx.prevPageLastElemOffset
);
if (isClip === false || k === 0) {
drawPaths.call(this, rule, isClip);
Expand Down Expand Up @@ -2007,28 +2034,34 @@ import { console } from "../libs/console.js";
sortPages(pages);

var clipPath, oldSize, oldLineWidth;
if (this.autoPaging === true) {
if (this.autoPaging) {
var min = pages[0];
var max = pages[pages.length - 1];
for (var i = min; i < max + 1; i++) {
this.pdf.setPage(i);

var topMargin = (i === 1 ? this.posY + this.margin[0] : this.margin[0]);
var firstPageHeight = this.pdf.internal.pageSize.height - this.posY - this.margin[0] - this.margin[2];
var pageHeightMinusMargin = this.pdf.internal.pageSize.height - this.margin[0] - this.margin[2];
var pageWidthMinusMargin = this.pdf.internal.pageSize.width - this.margin[1];
var previousPageHeightSum = i === 1 ? 0 : firstPageHeight + (i - 2) * pageHeightMinusMargin;

if (this.ctx.clip_path.length !== 0) {
var tmpPaths = this.path;
clipPath = JSON.parse(JSON.stringify(this.ctx.clip_path));
this.path = pathPositionRedo(
clipPath,
this.posX,
-1 * this.pdf.internal.pageSize.height * (i - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin
);
drawPaths.call(this, "fill", true);
this.path = tmpPaths;
}
var tmpRect = JSON.parse(JSON.stringify(textRect));
tmpRect = pathPositionRedo(
[tmpRect],
this.posX,
-1 * this.pdf.internal.pageSize.height * (i - 1) + this.posY
this.posX + this.margin[3],
-1 * previousPageHeightSum + topMargin + this.ctx.prevPageLastElemOffset
)[0];

if (options.scale >= 0.01) {
Expand All @@ -2037,12 +2070,24 @@ import { console } from "../libs/console.js";
oldLineWidth = this.lineWidth;
this.lineWidth = oldLineWidth * options.scale;
}
this.pdf.text(options.text, tmpRect.x, tmpRect.y, {
angle: options.angle,
align: textAlign,
renderingMode: options.renderingMode,
maxWidth: options.maxWidth
});

if (tmpRect.y <= pageHeightMinusMargin) {
if (tmpRect.y - tmpRect.h >= 0 && tmpRect.x <= pageWidthMinusMargin) {
var croppedText = this.pdf.splitTextToSize(options.text, options.maxWidth || pageWidthMinusMargin - tmpRect.x)[0];
this.pdf.text(croppedText, tmpRect.x, tmpRect.y, {
angle: options.angle,
align: textAlign,
renderingMode: options.renderingMode,
renderMaxWidthOverflow: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here is still the relict from the initial commit.

});
}
} else {
// This text is the last element of the page, but it got cut off due to the margin
// so we render it in the next page

// As a result, all other elements have their y offset increased
this.ctx.prevPageLastElemOffset += pageHeightMinusMargin - tmpRect.y + tmpRect.h;
}
Copy link
Contributor

@Rui-Jesus Rui-Jesus Nov 18, 2020

Choose a reason for hiding this comment

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

My previous comment regarding this code:

Both if conditions are not needed, they can be removed.
This is because getPagesByPath is already accounting for margins.
Also this code was falsely accusing out of bonds characters and when this happened, these characters would not be drawn(because the for loop through the pages only iterates once because there only is one page).

Was wrong, there are certain situations where one character can be attributed 2 pages (right on the edge of the bottom margin). The code was wrongfully pointing out characters as being out of range when they were not however and this is problematic when only one page was assigned to the character because in this situation the character will not be inserted in the pdf. This is because the comparison:
tmpRect.y <= pageHeightMinusMargin is not correct. tmpRect.y is a value that can range from 0 to the page height. pageHeightMinusMargin can range from 0 to page height minus the margins (both top and bottom margins). What happens when the page height is 200, top and bottom margins are 10 and the tmpRect.y is 185? The code will evaluate if 185 <= (200 -10 -10) which is false. But 185 fits in the page boundaries. Thus, the character will not get added to the page.

Suggested change
if (tmpRect.y <= pageHeightMinusMargin) {
if (tmpRect.y - tmpRect.h >= 0 && tmpRect.x <= pageWidthMinusMargin) {
var croppedText = this.pdf.splitTextToSize(options.text, options.maxWidth || pageWidthMinusMargin - tmpRect.x)[0];
this.pdf.text(croppedText, tmpRect.x, tmpRect.y, {
angle: options.angle,
align: textAlign,
renderingMode: options.renderingMode,
renderMaxWidthOverflow: false
});
}
} else {
// This text is the last element of the page, but it got cut off due to the margin
// so we render it in the next page
// As a result, all other elements have their y offset increased
this.ctx.prevPageLastElemOffset += pageHeightMinusMargin - tmpRect.y + tmpRect.h;
}
if ((this.margin[0] <= tmpRect.y) && (tmpRect.y <= (this.pdf.internal.pageSize.height - this.margin[2]))) {
var croppedText = this.pdf.splitTextToSize(options.text, options.maxWidth || pageWidthMinusMargin - tmpRect.x)[0];
this.pdf.text(croppedText, tmpRect.x, tmpRect.y, {
angle: options.angle,
align: textAlign,
renderingMode: options.renderingMode,
renderMaxWidthOverflow: false
});
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

To me, it looks like there is something severely broken. If you take the page break with text test case in the html.spec, the margins don't arrive in the context2d module at all and the the tmpRect.y coordinate for the first putText call with "Lorem " is completely wrong (its ~846, but should be something closer to 0, the y coordinate for the next text part "ipsum" is 33.9).

If I understand correctly, putText will always draw only a single line of text. So the pages array has either only one page, if the line bounds fit entirely on the current page, or two pages if the line bounds don't fit entirely. If the pages array has two pages, we should simply draw the text on the second page. In this case we also need to shift subsequent text further down.


if (options.scale >= 0.01) {
this.pdf.setFontSize(oldSize);
Expand All @@ -2056,7 +2101,7 @@ import { console } from "../libs/console.js";
oldLineWidth = this.lineWidth;
this.lineWidth = oldLineWidth * options.scale;
}
this.pdf.text(options.text, pt.x + this.posX, pt.y + this.posY, {
this.pdf.text(options.text, pt.x + this.posX + this.margin[3], pt.y + this.posY, {
angle: options.angle,
align: textAlign,
renderingMode: options.renderingMode,
Expand Down
1 change: 1 addition & 0 deletions src/modules/html.js
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ import { globalObject } from "../libs/globalObject.js";
pdf.context2d.autoPaging = true;
pdf.context2d.posX = this.opt.x;
pdf.context2d.posY = this.opt.y;
pdf.context2d.margin = this.opt.margin;

options.windowHeight = options.windowHeight || 0;
options.windowHeight =
Expand Down
Binary file added test/reference/html-margin-page-break-text.pdf
Binary file not shown.
Binary file added test/reference/html-margin-page-break.pdf
Binary file not shown.
Binary file added test/reference/html-margin-x-y.pdf
Binary file not shown.
Binary file added test/reference/html-margin.pdf
Binary file not shown.
Binary file added test/reference/html-x-y.pdf
Binary file not shown.
103 changes: 103 additions & 0 deletions test/specs/html.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,107 @@ describe("Module: html", function() {
);
comparePdf(doc.output(), "html-basic.pdf", "html");
});

it("html margin insets properly", async () => {
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
doc.line(30, 10, 100, 10);
doc.line(30, 10, 30, 100);
await new Promise(resolve =>
doc.html(
"<div style='background: red; width: 10px; height: 10px;'></div>",
{
callback: resolve,
margin: [10, 30]
}
)
);
comparePdf(doc.output(), "html-margin.pdf", "html");
});

it("html margin on page break", async () => {
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
doc.rect(
30,
10,
doc.internal.pageSize.getWidth() - 60,
doc.internal.pageSize.getHeight() - 20
);
await new Promise(resolve =>
doc.html(
"<div style='background: red; width: 10px; height: 1000px;'></div>",
{
callback: resolve,
margin: [10, 30, 10, 30]
}
)
);
doc.rect(
30,
10,
doc.internal.pageSize.getWidth() - 60,
doc.internal.pageSize.getHeight() - 20
);
comparePdf(doc.output(), "html-margin-page-break.pdf", "html");
});

it("html x, y offsets properly", async () => {
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
doc.line(30, 10, 100, 10);
doc.line(30, 10, 30, 100);
await new Promise(resolve =>
doc.html(
"<div style='background: red; width: 10px; height: 10px;'></div>",
{
callback: resolve,
x: 30,
y: 10
}
)
);
comparePdf(doc.output(), "html-x-y.pdf", "html");
});

it("html x, y + margin offsets properly", async () => {
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
doc.line(30, 10, 100, 10);
doc.line(30, 10, 30, 100);
await new Promise(resolve =>
doc.html(
"<div style='background: red; width: 10px; height: 10px;'></div>",
{
callback: resolve,
x: 10,
y: 3,
margin: [7, 20]
}
)
);
comparePdf(doc.output(), "html-margin-x-y.pdf", "html");
});

it("page break with text", async () => {
const doc = jsPDF({ floatPrecision: 2, unit: "pt" });
doc.rect(
30,
10,
doc.internal.pageSize.getWidth() - 60,
doc.internal.pageSize.getHeight() - 20
);
await new Promise(resolve =>
doc.html(
"<span>Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua. Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.</span>",
{
callback: resolve,
margin: [10, 30, 10, 30]
}
)
);
doc.rect(
30,
10,
doc.internal.pageSize.getWidth() - 60,
doc.internal.pageSize.getHeight() - 20
);
comparePdf(doc.output(), "html-margin-page-break-text.pdf", "html");
});
});