-
Notifications
You must be signed in to change notification settings - Fork 4.8k
Add XFA support to AcroForm module #3909
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this! There is one typo that is causing issues, otherwise LGTM! (Looking back at the originating issue, the typo was mine! 😳)
src/modules/acroform.js
Outdated
| " R" | ||
| ); | ||
| if (scope.internal.acroformPlugin.needRendering === true) { | ||
| scope.internal.write("/NeedRendering true"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be NeedsRendering (plural). Check and fix throughout PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching that! I’ve updated the code/tests to use /NeedsRendering and pushed the change.
8b46a68 to
58511ce
Compare
There was a problem hiding this 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. I'm no expert regarding XFA, but the code generally looks good. Could you add a test case that creates/compares a PDF with a reference, so I can check if the created PDF works.
src/modules/acroform.js
Outdated
| internal: null, | ||
| isInitialized: false | ||
| isInitialized: false, | ||
| needRendering: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also rename this field to "needsRendering"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review! I’ve renamed the internal flag to needsRendering and added a regression test in test/specs/acroform.spec.js (it("addXFA generates expected PDF")) that compares the generated file against the new reference test/reference/xfa-basic.pdf.
You can reproduce it with:
npm run test-unit -- --grep "addXFA generates expected PDF" --reporters mochaThe reference PDF contains /NeedsRendering true in the catalog and the /XFA [(datasets) … (template) …] packets (verified via qpdf --qdf --object-streams=disable test/reference/xfa-basic.pdf -). Happy to adjust anything else!
5f46167 to
b611e34
Compare
Summary
addXFAentry point to embed XFA payloads and toggle NeedRenderingTesting
Closes #3886
(Friendly note: I’m participating in Hacktoberfest—thanks for tagging or merging!)