Interactive forms: render choice widget annotations #7671

timvandermeij · 2016-09-25T17:29:10Z

This patch is another part of #7613.

The readOnly property has been moved to the WidgetAnnotation class as the specification says that all types of widget annotations may use this flag, and indeed we need it for choice widget annotations as well.
The fieldValue property is now only fetched in the WidgetAnnotation class and verification is done in the inheriting classes. This is required because for text widget annotations it's a string, whereas for choice widget annotations it may be an array as well.

timvandermeij · 2016-09-25T17:44:56Z

/botio test

pdfjsbot · 2016-09-25T17:45:11Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-09-25T18:14:28Z

From: Bot.io (Linux)

Failed

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

Total script time: 29.28 mins

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

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

timvandermeij · 2016-09-25T18:46:32Z

The bots received an update to Firefox 51, which causes a lot of reference test changes. I do wonder, however, why the Windows bot did not do anything...

Snuffleupagus

I'll try to do a proper review later in the week!
Taking a quick look at the PR I've noticed two things so far, please see the inline comment and the error below.

When opening annotation-choice-widget.pdf locally against master with forms disabled, I get the following error:

An error occurred while rendering the page.
PDF.js v1.5.488 (build: 6c263c1)
Message: str.charCodeAt is not a function
Stack: stringToPDFString@resource://pdf.js/build/pdf.worker.js:2035:42
WidgetAnnotation@resource://pdf.js/build/pdf.worker.js:40776:23
AnnotationFactory_create@resource://pdf.js/build/pdf.worker.js:40266:16
get annotations@resource://pdf.js/build/pdf.worker.js:41471:26
LocalPdfManager_ensure/<@resource://pdf.js/build/pdf.worker.js:41862:15
LocalPdfManager_ensure@resource://pdf.js/build/pdf.worker.js:41860:14
Page_getOperatorList@resource://pdf.js/build/pdf.worker.js:41389:32
wphSetupRenderPage/<@resource://pdf.js/build/pdf.worker.js:42660:9
Async*wphSetupRenderPage@resource://pdf.js/build/pdf.worker.js:42653:7
messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:2607:9
EventListener.handleEvent*MessageHandler@resource://pdf.js/build/pdf.worker.js:2613:3
wphCreateDocumentHandler@resource://pdf.js/build/pdf.worker.js:42301:19
wphSetupDoc@resource://pdf.js/build/pdf.worker.js:42288:14
messageHandlerComObjOnMessage/<@resource://pdf.js/build/pdf.worker.js:2584:18
Async*messageHandlerComObjOnMessage@resource://pdf.js/build/pdf.worker.js:2583:9
EventListener.handleEvent*MessageHandler@resource://pdf.js/build/pdf.worker.js:2613:3
initializeWorker@resource://pdf.js/build/pdf.worker.js:42812:17
pdfjsWrapper/<@resource://pdf.js/build/pdf.worker.js:42820:3
pdfjsWrapper/<@resource://pdf.js/build/pdf.worker.js:41975:5
pdfjsWrapper@resource://pdf.js/build/pdf.worker.js:41973:2
@resource://pdf.js/build/pdf.worker.js:40:13
@resource://pdf.js/build/pdf.worker.js:25:1
@resource://pdf.js/build/pdf.worker.js:18:2

Snuffleupagus · 2016-09-26T12:25:54Z

src/core/annotation.js

+    // it to an array of arrays as well for convenience in the display layer.
+    this.data.options = [];
+
+    var options = params.dict.get('Opt');


To prevent strange, and hard to find, bugs later on you probably want to use getArray here (or maybe var option = params.xref.fetchIfRef(options[i]); below).

Good find. I'll fix that when the comments from the full review are also present.

timvandermeij · 2016-09-26T20:30:34Z

The error is in fact a bug in the current master branch. As far as I can tell, it has been there since the moment the original widget code was added. The problem is that https://github.com/mozilla/pdf.js/pull/7671/files#diff-18ca06e2bd5be4c9132b51f78388b8f0L622 expects the V entry to be a string, but in the case of choice widgets it's an array, which leads to problems as stringToPDFString cannot handle that. This patch fixes that by moving the stringToPDFString conversion into TextWidgetAnnotation. Does that answer your question?

benweet · 2016-09-27T14:21:21Z

I've just tested this, it works beautifully. You can merge this before #7664 if you want, I can rebase and merge after that.

timvandermeij · 2016-10-04T11:58:10Z

@Snuffleupagus Do you have more review comments or can I start fixing this up with your current comments so we can land this?

Snuffleupagus · 2016-10-04T12:09:13Z

@Snuffleupagus Do you have more review comments or can I start fixing this up with your current comments so we can land this?

Really sorry for the delay here!
I had intended to test/review this properly during the weekend, but after seeing this dev.planning thread late on Friday I had a difficult time mustering up the energy required to do a proper review.

However, I'll try to review this either tonight or tomorrow; again sorry for the delay here!

timvandermeij · 2016-10-04T12:13:45Z

No worries! I also read that thread and it bugged me a bit as well, so you're not alone there... Thanks, I'll fix up this PR when the review comments are added.

Snuffleupagus

Again, sorry for the delay here! The PR itself looks good, however it exposes a couple of existing issues in the code; please see the inline comments.

r=me, with comments addressed and passing tests.

Snuffleupagus · 2016-10-05T11:06:58Z

test/unit/annotation_layer_spec.js

+      ]);
+
+      var choiceWidgetAnnotation = annotationFactory.create(xref,
+                                                            choiceWidgetRef);


Here, and in the other tests, you may also want to explicitly check that you actually get the expected annotation type, e.g. like this:

expect(data.annotationType).toEqual(AnnotationType.WIDGET);

Snuffleupagus · 2016-10-05T11:10:09Z

src/core/annotation.js

+    // Determine the field value. In this case, it may be a string or an
+    // array of strings. For convenience in the display layer, convert the
+    // string to an array of one string as well.
+    if (!isArray(this.data.fieldValue)) {


Similar to my other comment about references inside of arrays, if this.data.fieldValue already is an array you will have problems in the display layer if an array element is a Ref.

Please refer to the comment on the data.fieldValue = Util.getInheritableProperty(dict, 'V'); line for a suggested solution.

Snuffleupagus · 2016-10-05T11:19:01Z

src/core/annotation.js

@@ -619,8 +621,7 @@ var WidgetAnnotation = (function WidgetAnnotationClosure() {
    var data = this.data;

    data.annotationType = AnnotationType.WIDGET;
-    data.fieldValue = stringToPDFString(
-      Util.getInheritableProperty(dict, 'V') || '');
+    data.fieldValue = Util.getInheritableProperty(dict, 'V');


Since the V entry can be an array, you probably need to modify Util.getInheritableProperty to handle that. I can see two possible solutions:

Modify Util.getInheritableProperty to use always use Dict_getArray instead of Dict_get. Even though it only looks like this utility function is used from src/core/annotation.js, I'm not sure that this is such a great solution!

Add a parameter to Util.getInheritableProperty that controls whether Dict_getArray is used, e.g. something like this:

diff --git a/src/core/annotation.js b/src/core/annotation.js index 13aa204..d7ef2ca 100644 --- a/src/core/annotation.js +++ b/src/core/annotation.js @@ -619,8 +619,8 @@ var WidgetAnnotation = (function WidgetAnnotationClosure() { var data = this.data; data.annotationType = AnnotationType.WIDGET; - data.fieldValue = stringToPDFString( - Util.getInheritableProperty(dict, 'V') || ''); + data.fieldValue = Util.getInheritableProperty(dict, 'V', + /* getArray = */ true); data.alternativeText = stringToPDFString(dict.get('TU') || ''); data.defaultAppearance = Util.getInheritableProperty(dict, 'DA') || ''; var fieldType = Util.getInheritableProperty(dict, 'FT'); diff --git a/src/shared/util.js b/src/shared/util.js index 48876da..614e7a0 100644 --- a/src/shared/util.js +++ b/src/shared/util.js @@ -874,9 +874,10 @@ var Util = (function UtilClosure() { }; Util.getInheritableProperty = function Util_getInheritableProperty(dict, - name) { + name, + getArray) { while (dict && !dict.has(name)) { - dict = dict.get('Parent'); + dict = getArray ? dict.getArray('Parent') : dict.get('Parent'); } if (!dict) { return null;

Snuffleupagus · 2016-10-05T11:29:46Z

src/display/annotation_layer.js

+var ChoiceWidgetAnnotationElement = (
+    function ChoiceWidgetAnnotationElementClosure() {
+  function ChoiceWidgetAnnotationElement(parameters) {
+    WidgetAnnotationElement.call(this, parameters);


When we only supported TextWidgetAnnotations, I suppose it didn't matter too much that we set the isRenderable parameter inside of WidgetAnnotationElement, however now I don't think that's correct anymore!
Note: For TextWidgetAnnotation there's a meaningful fallback in the case where no Appearance stream existed, and was thus not rendered in the core layer, but for the ChoiceWidgetAnnotationElement we're not rendering anything in the display layer by default.

I believe that we should thus modify the current code like below, which will help not only here but when implementing other widget's later.

diff --git a/src/display/annotation_layer.js b/src/display/annotation_layer.js index 29ed79a..76a8b73 100644 --- a/src/display/annotation_layer.js +++ b/src/display/annotation_layer.js @@ -400,9 +400,7 @@ var TextAnnotationElement = (function TextAnnotationElementClosure() { * @alias WidgetAnnotationElement */ var WidgetAnnotationElement = (function WidgetAnnotationElementClosure() { - function WidgetAnnotationElement(parameters) { - var isRenderable = parameters.renderInteractiveForms || - (!parameters.data.hasAppearance && !!parameters.data.fieldValue); + function WidgetAnnotationElement(parameters, isRenderable) { AnnotationElement.call(this, parameters, isRenderable); } @@ -432,7 +430,9 @@ var TextWidgetAnnotationElement = ( var TEXT_ALIGNMENT = ['left', 'center', 'right']; function TextWidgetAnnotationElement(parameters) { - WidgetAnnotationElement.call(this, parameters); + var isRenderable = parameters.renderInteractiveForms || + (!parameters.data.hasAppearance && !!parameters.data.fieldValue); + WidgetAnnotationElement.call(this, parameters, isRenderable); } Util.inherit(TextWidgetAnnotationElement, WidgetAnnotationElement, {

Then this line becomes

WidgetAnnotationElement.call(this, parameters, parameters.renderInteractiveForms);

and the if (this.renderInteractiveForms) { condition is no longer necessary inside of render.

Snuffleupagus · 2016-10-05T11:30:13Z

src/display/annotation_layer.js

+    render: function ChoiceWidgetAnnotationElement_render() {
+      this.container.className = 'choiceWidgetAnnotation';
+
+      if (this.renderInteractiveForms) {


This if can be removed with the above comment addressed.

timvandermeij · 2016-10-05T19:36:22Z

/botio-linux preview

pdfjsbot · 2016-10-05T19:36:22Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/24712ec8981cf8d/output.txt

pdfjsbot · 2016-10-05T19:37:24Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/24712ec8981cf8d/output.txt

Total script time: 1.03 mins

Published

timvandermeij · 2016-10-05T19:37:49Z

/botio test

pdfjsbot · 2016-10-05T19:37:50Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/0e9f062377017fa/output.txt

pdfjsbot · 2016-10-05T19:37:50Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-10-05T20:03:33Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/0e9f062377017fa/output.txt

Total script time: 25.71 mins

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

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

pdfjsbot · 2016-10-05T20:06:24Z

From: Bot.io (Linux)

Failed

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

Total script time: 28.55 mins

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

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

Snuffleupagus · 2016-10-05T20:28:46Z

/botio makeref

pdfjsbot · 2016-10-05T20:28:46Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/6f9600046d338d9/output.txt

pdfjsbot · 2016-10-05T20:28:46Z

From: Bot.io (Linux)

Received

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

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

pdfjsbot · 2016-10-05T20:33:17Z

From: Bot.io (Windows)

Failed

Full output at http://107.22.172.223:8877/6f9600046d338d9/output.txt

Total script time: 4.50 mins

Lint: Passed
Make references: FAILED

timvandermeij · 2016-10-05T20:39:35Z

/botio-windows makeref

pdfjsbot · 2016-10-05T20:39:36Z

From: Bot.io (Windows)

Received

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

Live output at: http://107.22.172.223:8877/46aa938fc0e92bf/output.txt

pdfjsbot · 2016-10-05T20:50:34Z

From: Bot.io (Linux)

Failed

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

Total script time: 21.80 mins

Lint: Passed
Make references: FAILED

timvandermeij · 2016-10-05T20:59:13Z

/botio-linux makeref

pdfjsbot · 2016-10-05T20:59:13Z

From: Bot.io (Linux)

Received

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

Live output at: http://107.21.233.14:8877/55945b682744213/output.txt

pdfjsbot · 2016-10-05T21:04:55Z

From: Bot.io (Windows)

Success

Full output at http://107.22.172.223:8877/46aa938fc0e92bf/output.txt

Total script time: 25.30 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2016-10-05T21:27:34Z

From: Bot.io (Linux)

Success

Full output at http://107.21.233.14:8877/55945b682744213/output.txt

Total script time: 28.36 mins

Lint: Passed
Make references: Passed
Check references: Passed

…choice-fields Interactive forms: render choice widget annotations

timvandermeij added annotations form-acroform labels Sep 25, 2016

timvandermeij assigned Snuffleupagus Sep 25, 2016

timvandermeij mentioned this pull request Sep 25, 2016

Interactive form (AcroForm) support #7613

Closed

23 tasks

timvandermeij force-pushed the interactive-forms-choice-fields branch from 73ac9c0 to bcfe533 Compare September 25, 2016 17:37

Snuffleupagus requested changes Sep 26, 2016

View reviewed changes

Snuffleupagus requested changes Oct 5, 2016

View reviewed changes

timvandermeij added 2 commits October 5, 2016 21:25

Choice widget annotations: core and display layer implementation

d5d9f36

Choice widget annotations: unit and reference testing

f85f324

timvandermeij force-pushed the interactive-forms-choice-fields branch from bcfe533 to f85f324 Compare October 5, 2016 19:32

Snuffleupagus approved these changes Oct 5, 2016

View reviewed changes

timvandermeij merged commit 9b3a91f into mozilla:master Oct 5, 2016

timvandermeij deleted the interactive-forms-choice-fields branch October 5, 2016 21:27

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

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

7f2518e

…choice-fields Interactive forms: render choice widget annotations

Interactive forms: render choice widget annotations #7671

Interactive forms: render choice widget annotations #7671

Conversation

timvandermeij commented Sep 25, 2016 • edited Loading

timvandermeij commented Sep 25, 2016

pdfjsbot commented Sep 25, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Sep 25, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Sep 25, 2016

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Sep 26, 2016

Choose a reason for hiding this comment

timvandermeij Sep 26, 2016

Choose a reason for hiding this comment

timvandermeij commented Sep 26, 2016 • edited Loading

benweet commented Sep 27, 2016

timvandermeij commented Oct 4, 2016

Snuffleupagus commented Oct 4, 2016

timvandermeij commented Oct 4, 2016 • edited Loading

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 5, 2016

Choose a reason for hiding this comment

Snuffleupagus Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Oct 5, 2016

Choose a reason for hiding this comment

Snuffleupagus Oct 5, 2016 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Oct 5, 2016

Choose a reason for hiding this comment

timvandermeij commented Oct 5, 2016

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Success

Published

timvandermeij commented Oct 5, 2016

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Failed

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Failed

Snuffleupagus commented Oct 5, 2016

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Failed

timvandermeij commented Oct 5, 2016

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Failed

timvandermeij commented Oct 5, 2016

pdfjsbot commented Oct 5, 2016

From: Bot.io (Linux)

Received

pdfjsbot commented Oct 5, 2016

From: Bot.io (Windows)

Success

timvandermeij commented Sep 25, 2016 •

edited

Loading

timvandermeij commented Sep 26, 2016 •

edited

Loading

timvandermeij commented Oct 4, 2016 •

edited

Loading

Snuffleupagus Oct 5, 2016 •

edited

Loading

Snuffleupagus Oct 5, 2016 •

edited

Loading