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

Add support for textfied & choice widgets printing #12112

Closed
wants to merge 1 commit into from

Conversation

calixteman
Copy link
Contributor

No description provided.

@calixteman calixteman force-pushed the text branch 2 times, most recently from c2d741e to bbbf4d4 Compare July 23, 2020 13:06
@calixteman calixteman force-pushed the text branch 2 times, most recently from f2845fe to e832a80 Compare July 24, 2020 09:57
@lundjordan lundjordan added the Gecko 81 Tracking work planned for Form Fill in Shirley 81 release label Jul 27, 2020
@calixteman calixteman force-pushed the text branch 2 times, most recently from 054bd05 to 670e91a Compare July 30, 2020 12:45
@calixteman
Copy link
Contributor Author

@timvandermeij, ping

@brendandahl
Copy link
Contributor

Tim, if you don't have time, I free to review this as well.

@timvandermeij
Copy link
Contributor

timvandermeij commented Aug 4, 2020

Like the original forms PR this one is pretty big, making it difficult to oversee that all cases are handled and tested. Can we split off the multiline and comb field handling to a follow-up PR to limit the scope here so it's easier to review?

@@ -789,6 +789,10 @@ function stringToPDFString(str) {
return strBuf.join("");
}

function escapeString(str) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is escapeString necessary in this PR? It's not trivial to read, so if we really need it it would need a unit test like the other util.js functions.

renderForms,
annotationStorage

if (!this.data.hasText) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does hasText indicate that it's an editable widget? Maybe editable would be a better name then?


const fontInfo = await this.getFontData(evaluator, task);
const [font, fontName] = fontInfo;
let [, , fontSize] = fontInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks a bit odd. Can we at least use underscores for the unused parts? Otherwise let fontSize = fontInfo[2] would probably be more readable.

@calixteman
Copy link
Contributor Author

Like the original forms PR this one is pretty big, making it difficult to oversee that all cases are handled and tested. Can we split off the multiline and comb field handling to a follow-up PR to limit the scope here so it's easier to review?

Ok I splited the PR into 3: #12175, #12176 and #12177.
It'd be nice to review all of them and so I can fix problems asap.
We'd like to have print & save support in next Firefox nightly.

@timvandermeij
Copy link
Contributor

Thank you! I'll take care of reviewing them today. There has been quite a lot of activity lately for PDF.js, which is really good, but also takes up quite some time to get everything reviewed. The previous patches turned out really good though, so I have no doubt we can get these merged soon as well!

@timvandermeij
Copy link
Contributor

Closing this one in favor of the split up ones.

@timvandermeij timvandermeij removed annotations form-acroform Gecko 81 Tracking work planned for Form Fill in Shirley 81 release labels Aug 5, 2020
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.

4 participants