Add form printing support #12076

calixteman · 2020-07-08T13:38:10Z

This PR adds support to print forms:

textfields;
checkbox;
radio.
I actively tested with the form f1040.pdf (https://www.irs.gov/pub/irs-pdf/f1040.pdf) and it works pretty well.

timvandermeij

These comments are from an initial pass over the patch. Obviously it needs a more in depth review and more testing, but I must say that it looks a lot like what I had in mind for the printing support. Thank you for working on this!

web/app_options.js

src/display/canvas.js

web/annotation_storage.js

src/core/annotation.js

src/display/annotation_layer.js

src/core/annotation.js

src/display/api.js

test/unit/annotation_spec.js

web/annotation_layer_builder.js

web/app.js

src/core/annotation.js

calixteman · 2020-07-09T12:40:39Z

Thanks a lot for your review: I really appreciate.
I need to fix the default vertical padding and handle multilines text.

Snuffleupagus · 2020-07-09T14:36:51Z

src/core/annotation.js

+      task,
+      resources: this.fieldResources,
+      operatorList,
+      initialState,


Please note the default value in

pdf.js/src/core/evaluator.js

Lines 1224 to 1235 in 72d71ba

getOperatorList({

stream,

task,

resources,

operatorList,

initialState = null,

}) {

// Ensure that `resources`/`initialState` is correctly initialized,

// even if the provided parameter is e.g. `null`.

resources = resources || Dict.empty;

initialState = initialState || new EvalState();

hence you shouldn't need to pass the initialState here (nor export EvalState in the other file).

I removed EvalState export but I want to pass an initialState to be able to get the font and the font size from it in order to compute text width (and split it for multiline textfields).

If you want to chat about that or anything else, then I'm on riot in the pdf.js channel, my nick is "calixte" (in CEST timezone).

Sorry, I completely overlooked that you actually accessed the state afterwards; in that case exporting/using it like you previously did is probably OK.

(Although it does feel slightly "hacky" to do things in that way, given how the getOperatorList method in currently implemented/used, however at the top of my head I don't really have a much better suggestion.)

src/core/evaluator.js

Snuffleupagus · 2020-07-09T17:10:12Z

src/core/evaluator.js

@@ -3519,7 +3521,6 @@ class EvalState {
  constructor() {
    this.ctm = new Float32Array(IDENTITY_MATRIX);
    this.font = null;
-    this.fontSize = 0;


You definitely want to keep this line though, to ensure that there's always a meaningful default value.

(The changes in the handleSetFont method looks great, by the way.)

calixteman · 2020-07-21T16:30:08Z

@timvandermeij @Snuffleupagus, could you review this PR please ?

Snuffleupagus · 2020-07-21T17:16:54Z

src/core/annotation.js

+
+  async getFontData(evaluator, task) {
+    const operatorList = new OperatorList();
+    const initialState = new EvalState();


Really sorry, but I'm going to have to come back to my previous comments about how using an initialState in this way is (for a lack of a better word) "abusing" the getOperatorList method in ways that were never intended and why it unfortunately isn't guaranteed to always work.

It would actually be fairly easy to create a technically valid PDF document, where the intentState would still be empty even after parsing of the defaultAppearance has completed. Basically, simply wrapping the defaultAppearance in save/restore operators would break the code below which relies on initialState to contain the desired font properties (note how the StateManager, by necessity, deals with save/restore ops).

So, what could be a good way to get font and font size ?
Or maybe I can use a special state which clones to itself and then get the last used font from it, wdyt ?

So, what could be a good way to get font and font size ?

I honestly don't know, off the top of my head, since I don't believe that the getOperatorList functionality was really created for the particular use-cases you have mind here.

Or maybe I can use a special state which clones to itself and then get the last used font from it, wdyt ?

Assuming it won't negatively impact performance of the "normal" PDF document parsing, that could possibly work. (Although it still, to me, feels somewhat unfortunate that we're having to resort to these kind of "tricks" :-)

Of course, you might also want to see if @brendandahl has any better ideas (than me).

timvandermeij

Nice work so far!

This has become quite a big patch now compared to how it started, which makes sense because it developed along the way, but it makes it hard to make sure everything is alright in review. If this works correctly, we should split it up into multiple smaller PRs that we can more easily review and test/merge. In particular because we have no good way of testing printing other than manually, it would be nice to at least have unit tests for the parts that we can test. Since that would make the patch even bigger, I'm thinking splitting it is a good idea. For example, we could split this up into three PRs:

One PR that only introduces the storage, with unit tests, and passes it to the necessary components, but don't actually do anything with it yet.
One PR that basically contains the changes in src/core/annotation.js since they should be self-contained, with unit tests because that should be relatively easy to unit tests (see the existing unit tests for this).
Finally, one PR that basically contains the changes in src/display/annotation_layer.js and the remaining changes.

However, it might make more sense to make the split per field type:

One PR for the storage with unit tests again.
One PR for the core/display layer changes for radio buttons.
One PR like the above but for checkboxes.
Finally, one PR for the above but for text fields.

That way it's much more self-contained and makes merging much quicker since it allows us to better oversee the changes, especially with good unit test coverage in place.

timvandermeij · 2020-07-21T21:48:59Z

web/firefox_print_service.js

@@ -109,7 +110,7 @@ PDFPrintServiceFactory.instance = {
    return shadow(this, "supportsPrinting", value);
  },

-  createPrintService(pdfDocument, pagesOverview, printContainer) {
+  createPrintService(pdfDocument, pagesOverview, printContainer, l10n) {


I don't really see why this change is necessary?

timvandermeij · 2020-07-21T21:52:41Z

src/core/annotation.js

+  }
+}
+
+class WidgetWithTextAnnotation extends WidgetAnnotation {


The classes in this file directly match the specification which makes it useful to map it one-to-one. I would therefore not introduce a new base class here, mainly because it adds yet another level of inheritance which makes it harder to follow, but also because this is not a class that the specification contains. Instead, we can simply put these methods on the WidgetAnnotation class itself and overwrite them in subclasses if necessary. This way we have one less level of inheritance while still keeping the functionality.

calixteman · 2020-07-23T13:15:58Z

I splited this PR in multiple ones:
#12106
#12107
#12108
#12112

timvandermeij · 2020-07-24T21:50:41Z

Closing this one in favor of the split up PRs instead.

Add form printing support

5f61c6e

timvandermeij added form-acroform printing annotations labels Jul 8, 2020

timvandermeij reviewed Jul 8, 2020

View reviewed changes

Add form printing support for Firefox

13f7dff

Snuffleupagus requested changes Jul 9, 2020

View reviewed changes

Snuffleupagus reviewed Jul 9, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

Snuffleupagus reviewed Jul 9, 2020

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

calixteman added 2 commits July 9, 2020 14:28

Address nits

e8542dc

Forgot to disable renderInteractiveForms

541ad5f

Default text rise is now based on font.descent

8ef0492

Snuffleupagus reviewed Jul 9, 2020

View reviewed changes

src/core/evaluator.js Outdated Show resolved Hide resolved

calixteman added 2 commits July 9, 2020 18:18

Handle multiline text

c6a4c1e

Don't export EvalState

ce18017

Snuffleupagus reviewed Jul 9, 2020

View reviewed changes

calixteman added 7 commits July 9, 2020 19:14

Export EvalState and use it to get font

64f6e17

Font descent can be NaN

020ffd0

By default a textfield is left aligned

e7e11dc

Handle auto-sized font in textfields

f13252a

Don't leak password if any

2d11d4c

Fix tests after default text alignment change

3fe52eb

Handle choice widget

a8c553f

Snuffleupagus reviewed Jul 21, 2020

View reviewed changes

timvandermeij reviewed Jul 21, 2020

View reviewed changes

timvandermeij closed this Jul 24, 2020

lundjordan added the Gecko 81 Tracking work planned for Form Fill in Shirley 81 release label Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add form printing support #12076

Add form printing support #12076

calixteman commented Jul 8, 2020 •

edited

Loading

timvandermeij left a comment

calixteman commented Jul 9, 2020

Snuffleupagus Jul 9, 2020

calixteman Jul 9, 2020

calixteman Jul 9, 2020

Snuffleupagus Jul 9, 2020

Snuffleupagus Jul 9, 2020

calixteman commented Jul 21, 2020

Snuffleupagus Jul 21, 2020 •

edited

Loading

calixteman Jul 21, 2020

Snuffleupagus Jul 21, 2020 •

edited

Loading

timvandermeij left a comment

timvandermeij Jul 21, 2020

timvandermeij Jul 21, 2020

calixteman commented Jul 23, 2020

timvandermeij commented Jul 24, 2020

	getOperatorList({
	stream,
	task,
	resources,
	operatorList,
	initialState = null,
	}) {
	// Ensure that `resources`/`initialState` is correctly initialized,
	// even if the provided parameter is e.g. `null`.
	resources = resources \|\| Dict.empty;
	initialState = initialState \|\| new EvalState();

Add form printing support #12076

Add form printing support #12076

Conversation

calixteman commented Jul 8, 2020 • edited Loading

timvandermeij left a comment

Choose a reason for hiding this comment

calixteman commented Jul 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Jul 21, 2020

Snuffleupagus Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus Jul 21, 2020 • edited Loading

Choose a reason for hiding this comment

timvandermeij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calixteman commented Jul 23, 2020

timvandermeij commented Jul 24, 2020

calixteman commented Jul 8, 2020 •

edited

Loading

Snuffleupagus Jul 21, 2020 •

edited

Loading

Snuffleupagus Jul 21, 2020 •

edited

Loading