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

Feature/add rgba array support #3124

Merged

Conversation

Pantura
Copy link
Contributor

@Pantura Pantura commented Mar 24, 2021

This PR adds support for RGBA buffer input. This is the format that is output by DOM canvas getImageData but could be generated elsewhere.

Comment on lines +67 to +68
var rgbData = this.__addimage__.arrayBufferToBinaryString(rgbOut);
var alphaData = this.__addimage__.arrayBufferToBinaryString(alphaOut);
Copy link
Contributor

Choose a reason for hiding this comment

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

I assumed these accepted ArrayBuffer, not Uint8Array given the name...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not give enough thought to these before but the other code paths call it the same way.
If it is a buffer, make it an array, if it is an array, run it through arrayBufferToBinaryString.
https://github.com/MrRio/jsPDF/blob/master/src/modules/jpeg_support.js#L71-L83

All usage should be checked and name changed if that is uniform across all usage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Naming of the arrayBufferToBinaryString should imo be changed on a separate PR.

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 this PR :)

src/modules/addimage.js Show resolved Hide resolved
test/specs/rgba.spec.js Show resolved Hide resolved
test/specs/rgba.spec.js Show resolved Hide resolved
Comment on lines +64 to +82
it("with alpha", () => {
const c = document.createElement("canvas");
const ctx = c.getContext("2d");
ctx.fillStyle = "#FFFFFF";
ctx.fillRect(0, 0, 150, 60);
ctx.fillStyle = "#AA00FF77";
ctx.fillRect(10, 10, 130, 40);
const dataFromCanvas = ctx.getImageData(0, 0, 150, 60);

const doc = new jsPDF({
orientation: "p",
unit: "px",
format: "a4",
floatPrecision: 2
});
doc.addImage(dataFromCanvas, 10, 10, 150, 60);

comparePdf(doc.output(), "rgba_alpha.pdf", "addimage");
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like this test case produces slightly different results in IE. Could you investigate why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it would take some time to setup a windows test environment myself, I'm going throw a question: is IE11 really something that needs to be supported still? Microsoft itself tries to migrate everyone to Edge as fast as possible with all the IE problems and security issues being abundant.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right. IE is probably not that important anymore. I think it's OK, if we drop IE support for new features. So let's just skip this test case in IE.

# Conflicts:
#	types/jspdf-tests.ts
@HackbrettXXX HackbrettXXX merged commit b6ed102 into parallax:master Sep 9, 2021
@Pantura
Copy link
Contributor Author

Pantura commented Sep 9, 2021

👍

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.

None yet

4 participants