Text widget annotations: support read-only/multiline fields and improve testing #7633

timvandermeij · 2016-09-14T20:13:45Z

This pull request is a part of #7613 and completes the support for text widget annotations. More details are provided in the commit messages.

Snuffleupagus · 2016-09-15T09:46:43Z

src/core/annotation.js

-  create: function AnnotationFactory_create(xref, ref,
-                                            uniquePrefix, idCounters) {
+  create: function AnnotationFactory_create(xref, ref, uniquePrefix,
+                                            idCounters) {


Nit: Since you're not making any functional changes here, please revert this to avoid unnecessarily changing the blame of these lines.

Done in the new commits.

Snuffleupagus · 2016-09-15T09:48:10Z

test/test_manifest.json

@@ -3137,6 +3137,13 @@
       "type": "eq",
       "annotations": true
    },
+    {  "id": "annotation-text-widget",


I'd suggest adding -forms to the end of the test name, to make it obvious what's beeing tested.

Done in the new commits.

Snuffleupagus · 2016-09-15T09:54:10Z

src/core/annotation.js

-                                            idCounters) {
+  create:
+      function AnnotationFactory_create(xref, ref, uniquePrefix, idCounters,
+                                        renderInteractiveForms) {


Nit: I think that you could just do this instead (note the previous comment about changing the blame):

create: function AnnotationFactory_create(xref, ref, uniquePrefix, idCounters, renderInteractiveForms) {

Done in the new commits.

Snuffleupagus · 2016-09-15T10:01:15Z

src/display/api.js

+          intent: renderingIntent,
+          renderInteractiveForms:
+            (params && params.renderInteractiveForms === true ? true :
+             /* Default */ false),


I think that readability would be improved here if you instead defined an intermediate renderInteractiveForms variable above, similar to the renderingIntent. E.g.

var renderInteractiveForms = (params.renderInteractiveForms === true ? true : /* Default */ false); ... this.transport.messageHandler.send('RenderPageRequest', { pageIndex: this.pageNumber - 1, intent: renderingIntent, renderInteractiveForms: renderInteractiveForms, });

Done in the new commits.

Snuffleupagus

I've not yet had time to actually test this, but it looks good so far :-)
I've added a few comments, based on just browsing through the code, and I will do a more thorough review/test later.

Snuffleupagus

r=me, with comments addressed and passing tests.
Looks great, thank you!

Snuffleupagus · 2016-09-17T08:19:20Z

web/annotation_layer_builder.css

+  cursor: not-allowed;
+}
+
+.annotationLayer .textWidgetAnnotation textarea {


Shouldn't a textarea have the same hover/focus rules as a normal input!?
Because currently the background colour isn't removed when editing its contents, and there's no outline when the cursor is over it either.

Done in the new commit. I thought I had this, but it somehow got lost. Thanks for noticing!

Snuffleupagus · 2016-09-17T09:15:17Z

As a follow-up PR, I think that we should get rid of PDFJS.renderInteractiveForms as soon as possible.
In hindsight we should not have added it in the first place, since using these global variables is making it more difficult to have multiple PDFJS instances running at the same time.
Considering that the intention is to remove these global variables in the future, we really shouldn't be adding any more of them at this time!

Since we've only just added PDFJS.renderInteractiveForms, and considering that it's not used by default yet, I think that we can simply remove it without worrying about it.
Instead, let's pass in renderInteractiveForms to PDFViewer/PDFPageView/AnnotationLayerBuilder from app.js in the same way as we currently do with enhanceTextSelection.
I'll happily review a follow-up PR that fixes this :-)

Moreover, this patch provides us with a framework for handling field flags in general for all types of widget annotations.

This patch improves the unit tests by testing the support for read-only and multiline fields. Moreover, we add a reference test to ensure that the text widgets are not only rendered, but also that their contents are styled properly. Finally, we perform minor improvements in `src/core/annotation.js`, for example adding missing comments.

If interactive forms are enabled, then the display layer takes care of rendering the form elements. There is no need to draw them on the canvas as well. This also leads to issues when values are prefilled, because the text fields are transparent, so the contents that have been rendered onto the canvas will be visible too. We address this issue by passing the `renderInteractiveForms` parameter to the render task and handling it when the page is rendered (i.e., when the canvas is rendered).

timvandermeij · 2016-09-17T13:37:07Z

I have added a note to the tracking issue about removing the global.

timvandermeij · 2016-09-17T13:37:13Z

/botio-linux preview

pdfjsbot · 2016-09-17T13:37:13Z

From: Bot.io (Linux)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/c6e8f5c41558f14/output.txt

pdfjsbot · 2016-09-17T13:38:25Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/c6e8f5c41558f14/output.txt

Total script time: 1.19 mins

Published

timvandermeij · 2016-09-17T13:38:39Z

/botio test

pdfjsbot · 2016-09-17T13:38:39Z

From: Bot.io (Linux)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/e1f58e97e0a6772/output.txt

pdfjsbot · 2016-09-17T13:38:39Z

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/efe42373b55163f/output.txt

pdfjsbot · 2016-09-17T14:03:09Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/efe42373b55163f/output.txt

Total script time: 24.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://107.22.172.223:8877/efe42373b55163f/reftest-analyzer.html#web=eq.log

pdfjsbot · 2016-09-17T14:18:19Z

From: Bot.io (Linux)

Failed

Full output at http://107.21.233.14:8877/e1f58e97e0a6772/output.txt

Total script time: 39.66 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://107.21.233.14:8877/e1f58e97e0a6772/reftest-analyzer.html#web=eq.log

timvandermeij · 2016-09-17T14:41:05Z

I verified that the test failure above is correct. This field is actually a multiline text field that is positioned one pixel below the other field next to it. Now that this is supported, we notice this. Comparing a screenshot from the Adobe Reader rendering to the new PDF.js rendering shows that it is the same now.

/botio makeref

pdfjsbot · 2016-09-17T14:41:05Z

From: Bot.io (Windows)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.22.172.223:8877/9c4ce4251659d10/output.txt

pdfjsbot · 2016-09-17T14:41:05Z

From: Bot.io (Linux)

Received

Command cmd_makeref from @timvandermeij received. Current queue size: 0

Live output at: http://107.21.233.14:8877/b098952aec45cb8/output.txt

pdfjsbot · 2016-09-17T15:05:35Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/9c4ce4251659d10/output.txt

Total script time: 24.50 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2016-09-17T15:19:33Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/b098952aec45cb8/output.txt

Total script time: 38.45 mins

Lint: Passed
Make references: Passed
Check references: Passed

…tx-flags Text widget annotations: support read-only/multiline fields and improve testing

timvandermeij added the annotations label Sep 14, 2016

timvandermeij assigned Snuffleupagus Sep 14, 2016

timvandermeij force-pushed the interactive-forms-tx-flags branch 2 times, most recently from bc96b57 to 3fb2e39 Compare September 14, 2016 21:58

Snuffleupagus reviewed Sep 15, 2016

View reviewed changes

Snuffleupagus requested changes Sep 15, 2016

View reviewed changes

timvandermeij force-pushed the interactive-forms-tx-flags branch from 3fb2e39 to 291ac7e Compare September 15, 2016 13:50

timvandermeij added the form-acroform label Sep 15, 2016

Snuffleupagus approved these changes Sep 17, 2016

View reviewed changes

timvandermeij mentioned this pull request Sep 17, 2016

Interactive form (AcroForm) support #7613

Closed

23 tasks

timvandermeij added 3 commits September 17, 2016 15:24

Text widget annotations: support multiline and read-only fields

f6965fa

Moreover, this patch provides us with a framework for handling field flags in general for all types of widget annotations.

timvandermeij force-pushed the interactive-forms-tx-flags branch from 291ac7e to dbea302 Compare September 17, 2016 13:31

timvandermeij merged commit f062695 into mozilla:master Sep 17, 2016

timvandermeij deleted the interactive-forms-tx-flags branch September 17, 2016 15:19

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#7633 from timvandermeij/interactive-forms-…

304e641

…tx-flags Text widget annotations: support read-only/multiline fields and improve testing

Text widget annotations: support read-only/multiline fields and improve testing #7633

Text widget annotations: support read-only/multiline fields and improve testing #7633

Uh oh!

Conversation

timvandermeij commented Sep 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Snuffleupagus Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

timvandermeij Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

timvandermeij Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

timvandermeij Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus Sep 15, 2016

Choose a reason for hiding this comment

Uh oh!

timvandermeij Sep 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus left a comment

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus left a comment

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timvandermeij Sep 17, 2016

Choose a reason for hiding this comment

Uh oh!

Snuffleupagus commented Sep 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

timvandermeij commented Sep 17, 2016

Uh oh!

timvandermeij commented Sep 17, 2016

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Linux)

Received

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Linux)

Success

Published

Uh oh!

timvandermeij commented Sep 17, 2016

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Linux)

Received

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Windows)

Received

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Windows)

Failed

Uh oh!

pdfjsbot commented Sep 17, 2016

From: Bot.io (Linux)

timvandermeij commented Sep 14, 2016 •

edited

Loading

timvandermeij Sep 15, 2016 •

edited

Loading

timvandermeij Sep 15, 2016 •

edited

Loading

timvandermeij Sep 15, 2016 •

edited

Loading

timvandermeij Sep 15, 2016 •

edited

Loading

Snuffleupagus Sep 17, 2016 •

edited

Loading

Snuffleupagus commented Sep 17, 2016 •

edited

Loading