[Form] Don't use field appearances when /NeedAppearances is set to true (bug 1796741) #15615

calixteman · 2022-10-24T13:37:11Z

When a form isn't changed, we used the appearances we had in the file, but when /NeedAppearances is true, all the appearances have to be regenerated whatever they're.

Snuffleupagus · 2022-10-24T13:44:30Z

I can't access the bug, can you please CC me on it so that I have the complete context here?

Snuffleupagus · 2022-10-24T14:02:19Z

src/core/document.js

-              return null;
-            })
-          );
+    const acroFormPromise = this.pdfManager.ensureCatalog("acroForm");


With the comment above addressed, do we actually need to make any changes in this file?

Snuffleupagus · 2022-10-24T14:14:08Z

src/core/annotation.js

@@ -1480,6 +1491,10 @@ class WidgetAnnotation extends Annotation {
  constructor(params) {
    super(params);

+    if (params.needAppearances) {
+      this.appearance = null;


Could we break any other code-path, e.g. related to saving or similar, if we simply clear out the this.appearance data here?
Since I don't know the annotation-related code well enough to tell right away, I figured that it couldn't hurt to ask.

Normally no.
When the appearance is null, a new one should be generated from the field value..

Snuffleupagus · 2022-10-24T14:25:08Z

src/core/annotation.js

@@ -138,6 +148,7 @@ class AnnotationFactory {
      attachments,
      xfaDatasets,
      collectFields,
+      needAppearances,


Given

pdf.js/src/core/annotation.js

Line 79 in 987062c

pdfManager.ensureCatalog("acroForm"),

above, could we not simply do something like the following here instead?

Suggested change

needAppearances,

needAppearances: (acroForm instanceof Dict && acroForm.get("NeedAppearances)) || false,

Snuffleupagus

r=me, with passing tests; thank you!

calixteman · 2022-10-24T15:33:13Z

/botio test

pdfjsbot · 2022-10-24T15:33:15Z

From: Bot.io (Linux m4)

Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.241.84.105:8877/2f856d9e3d7e740/output.txt

pdfjsbot · 2022-10-24T15:33:15Z

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/ec8896a3056a300/output.txt

pdfjsbot · 2022-10-24T16:00:48Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/2f856d9e3d7e740/output.txt

Total script time: 3.92 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: Passed
Regression tests: FAILED

Image differences available at: http://54.241.84.105:8877/2f856d9e3d7e740/reftest-analyzer.html#web=eq.log

calixteman · 2022-10-24T16:07:01Z

/botio test

pdfjsbot · 2022-10-24T16:07:03Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/cf49613e5a6475b/output.txt

pdfjsbot · 2022-10-24T16:07:03Z

From: Bot.io (Windows)

Received

Command cmd_test from @calixteman received. Current queue size: 1

Live output at: http://54.193.163.58:8877/af3b00702ab3e94/output.txt

pdfjsbot · 2022-10-24T16:08:51Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/ec8896a3056a300/output.txt

Total script time: 7.17 mins

Font tests: Passed
Unit tests: FAILED
Integration Tests: FAILED
Regression tests: FAILED

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

pdfjsbot · 2022-10-24T16:32:00Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/cf49613e5a6475b/output.txt

Total script time: 24.95 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 17
  different first/second rendering: 1

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

pdfjsbot · 2022-10-24T16:38:49Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/af3b00702ab3e94/output.txt

Total script time: 29.97 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 7

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

calixteman · 2022-10-24T17:32:27Z

/botio test

pdfjsbot · 2022-10-24T17:32:28Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/d80fe200618527a/output.txt

pdfjsbot · 2022-10-24T17:32:28Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/78fbc63a7b218e9/output.txt

pdfjsbot · 2022-10-24T17:57:18Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/d80fe200618527a/output.txt

Total script time: 24.81 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 18
  different first/second rendering: 1

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

pdfjsbot · 2022-10-24T18:01:32Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/78fbc63a7b218e9/output.txt

Total script time: 29.04 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 7

Image differences available at: http://54.193.163.58:8877/78fbc63a7b218e9/reftest-analyzer.html#web=eq.log

src/core/annotation.js

Snuffleupagus

Revoking approval, while there's still ref-test regressions.

Snuffleupagus · 2022-10-25T14:06:32Z

src/core/annotation.js

      attachments,
      xfaDatasets,
      collectFields,
+      needAppearances:
+        !collectFields && (acroFormDict.get("NeedAppearances") || false),


Given that this should be a boolean value, can we please check explicitly for that to prevent any future surprises/bugs in corrupt documents?
For example, what if the entry happens to be a non-empty string or any other truthy value?

Suggested change

!collectFields && (acroFormDict.get("NeedAppearances") || false),

!collectFields && acroFormDict.get("NeedAppearances") === true,

calixteman · 2022-10-25T14:31:07Z

/botio test

Snuffleupagus · 2022-10-25T16:30:23Z

It's because of the test itself:

Sorry, but that still doesn't explain why it can be reproduced in the viewer with annotationMode = 1 set!?

calixteman · 2022-10-25T16:32:32Z

It's because of the test itself:

Sorry, but that still doesn't explain why it can be reproduced in the viewer with annotationMode = 1 set!?

Very likely because of:

pdf.js/src/display/api.js

Line 2354 in 098ca67

break;

Snuffleupagus · 2022-10-25T16:37:43Z

I still don't at all understand why these changes lead to the double rendering here, and I don't think it's correct to just leave this as-is.

(And this PR now looks way to risky to land less than a week before the next release...)

calixteman · 2022-10-25T16:41:25Z

The annotation is rendered one time in the annotationLayer and a second time in the canvas because the intent parameter in the getOperatorList function doesn't contain the flag RenderingIntentFlag.ANNOTATIONS_FORMS.

calixteman · 2022-10-25T16:44:26Z

The pdf has the flag /NeedAppearances set to true which means that the appearance for this form field must be generated based on its value, it's why there is something to render.
The stream is null here, so it's why, before the patch, an empty appearance was "rendered" on the canvas.

calixteman · 2022-10-25T16:48:19Z

Maybe the fix is just to add RenderingIntentFlag.ANNOTATIONS_FORMS in the intent when we've an annotation layer.
But I think we should do that in a follow-up.

Snuffleupagus · 2022-10-25T16:55:50Z

The annotation is rendered one time in the annotationLayer

Based on a brief look the solution is probably to update the hasAppearance parameter accordingly when /NeedAppearances is set, since that parameter is no longer correct in that case.

Maybe the fix is just to add RenderingIntentFlag.ANNOTATIONS_FORMS in the intent when we've an annotation layer.
But I think we should do that in a follow-up.

No, it must be possible to render annotations without also enabling forms (this was the behaviour for years).

calixteman · 2022-10-25T17:13:39Z

/botio test

pdfjsbot · 2022-10-25T17:13:41Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/bf7b8f4607e84bf/output.txt

pdfjsbot · 2022-10-25T17:13:41Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/3e9a664a01f42b2/output.txt

pdfjsbot · 2022-10-25T17:38:46Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/bf7b8f4607e84bf/output.txt

Total script time: 25.07 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 19
  different first/second rendering: 1

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

pdfjsbot · 2022-10-25T17:43:29Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/3e9a664a01f42b2/output.txt

Total script time: 29.78 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 7

Image differences available at: http://54.193.163.58:8877/3e9a664a01f42b2/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2022-10-26T09:54:34Z

src/core/annotation.js

@@ -3020,18 +3036,26 @@ class ChoiceWidgetAnnotation extends WidgetAnnotation {
      return super._getAppearance(evaluator, task, annotationStorage);
    }

-    if (!annotationStorage) {
+    if (!annotationStorage && this.appearance) {


Won't this cause an error to be thrown below if annotationStorage === undefined and this.appearance === undefined (or this.appearance === null)?

Yep, you're right, thanks for catching that.
I updated to have something similar to what we have in the appearance getter for text fields.

…ue (bug 1796741) When a form isn't changed, we used the appearances we had in the file, but when /NeedAppearances is true, all the appearances have to be regenerated whatever they're.

calixteman · 2022-10-26T10:12:53Z

/botio test

pdfjsbot · 2022-10-26T10:12:55Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/353591a9847c29b/output.txt

pdfjsbot · 2022-10-26T10:12:55Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/537d5f92a158de0/output.txt

pdfjsbot · 2022-10-26T10:38:10Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/353591a9847c29b/output.txt

Total script time: 25.23 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 16
  different first/second rendering: 1

Image differences available at: http://54.241.84.105:8877/353591a9847c29b/reftest-analyzer.html#web=eq.log

pdfjsbot · 2022-10-26T10:43:25Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/537d5f92a158de0/output.txt

Total script time: 30.48 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 7

Image differences available at: http://54.193.163.58:8877/537d5f92a158de0/reftest-analyzer.html#web=eq.log

Snuffleupagus

Hopefully we've now fixed all problems here, since the remaining movement in the tests should be expected/improvements.
Given that the double rendering has now been fully understood, and that the fix was pretty simple and straightforward I'm less worried about that.

Your call whether to land this now, or wait until after the upcoming release from the 3-branch.

calixteman · 2022-10-31T08:58:35Z

/botio makeref

pdfjsbot · 2022-10-31T08:58:37Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/769f9c638febd8a/output.txt

pdfjsbot · 2022-10-31T08:58:37Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/404000e086d6906/output.txt

pdfjsbot · 2022-10-31T09:19:51Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/769f9c638febd8a/output.txt

Total script time: 21.22 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-10-31T09:21:52Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/404000e086d6906/output.txt

Total script time: 23.23 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman added the form-acroform label Oct 24, 2022

calixteman requested a review from Snuffleupagus October 24, 2022 13:37

calixteman force-pushed the bug1796741 branch from 203cd6d to 18a9b49 Compare October 24, 2022 13:51

Snuffleupagus reviewed Oct 24, 2022

View reviewed changes

calixteman force-pushed the bug1796741 branch from 18a9b49 to 6221f7f Compare October 24, 2022 14:34

Snuffleupagus approved these changes Oct 24, 2022

View reviewed changes

calixteman force-pushed the bug1796741 branch from 6221f7f to c8360d0 Compare October 24, 2022 16:06

calixteman force-pushed the bug1796741 branch from c8360d0 to 3e08113 Compare October 24, 2022 17:32

Snuffleupagus reviewed Oct 25, 2022

View reviewed changes

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

calixteman force-pushed the bug1796741 branch from 3e08113 to 8a6acbf Compare October 25, 2022 13:58

Snuffleupagus requested changes Oct 25, 2022

View reviewed changes

calixteman force-pushed the bug1796741 branch from 8a6acbf to b650c4e Compare October 25, 2022 14:30

calixteman force-pushed the bug1796741 branch from b650c4e to 38d0593 Compare October 25, 2022 16:33

calixteman force-pushed the bug1796741 branch from 38d0593 to 755e6ce Compare October 25, 2022 17:13

Snuffleupagus reviewed Oct 26, 2022

View reviewed changes

[Form] Don't use field appearances when /NeedAppearances is set to tr…

9f95a14

…ue (bug 1796741) When a form isn't changed, we used the appearances we had in the file, but when /NeedAppearances is true, all the appearances have to be regenerated whatever they're.

calixteman force-pushed the bug1796741 branch from 755e6ce to 9f95a14 Compare October 26, 2022 10:11

Snuffleupagus approved these changes Oct 26, 2022

View reviewed changes

calixteman merged commit e42e1cd into mozilla:master Oct 31, 2022

Snuffleupagus mentioned this pull request Nov 1, 2022

Annotation - Use default values instead of default appearance when printing #13523

Closed

	needAppearances,
	needAppearances: (acroForm instanceof Dict && acroForm.get("NeedAppearances)) \|\| false,

	!collectFields && (acroFormDict.get("NeedAppearances") \|\| false),
	!collectFields && acroFormDict.get("NeedAppearances") === true,

[Form] Don't use field appearances when /NeedAppearances is set to true (bug 1796741) #15615

[Form] Don't use field appearances when /NeedAppearances is set to true (bug 1796741) #15615

Conversation

calixteman commented Oct 24, 2022

Snuffleupagus commented Oct 24, 2022 • edited Loading

Snuffleupagus Oct 24, 2022

Choose a reason for hiding this comment

Snuffleupagus Oct 24, 2022

Choose a reason for hiding this comment

calixteman Oct 24, 2022

Choose a reason for hiding this comment

Snuffleupagus Oct 24, 2022 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Oct 24, 2022

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Failed

calixteman commented Oct 24, 2022

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Failed

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Failed

calixteman commented Oct 24, 2022

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 24, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 24, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus Oct 25, 2022

Choose a reason for hiding this comment

calixteman commented Oct 25, 2022

Snuffleupagus commented Oct 25, 2022

calixteman commented Oct 25, 2022

Snuffleupagus commented Oct 25, 2022

calixteman commented Oct 25, 2022

calixteman commented Oct 25, 2022 • edited Loading

calixteman commented Oct 25, 2022

Snuffleupagus commented Oct 25, 2022

calixteman commented Oct 25, 2022

pdfjsbot commented Oct 25, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 25, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Oct 25, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Oct 25, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus Oct 26, 2022 • edited Loading

Snuffleupagus commented Oct 24, 2022 •

edited

Loading

Snuffleupagus Oct 24, 2022 •

edited

Loading

calixteman commented Oct 25, 2022 •

edited

Loading

Snuffleupagus Oct 26, 2022 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading