Render not displayed annotations in using normal appearance when printing #12395

calixteman · 2020-09-19T18:16:31Z

A pdf with several pages containing some text or checked radio/checkbox not on page 1/2 were not printed.
Since they were not rendered in annotation_layer, then the annotationStorage has no entries for such widget.
So the goal of the patch is to fix that in using normalAppearance when the annotationStorage doesn't contain the widget id.

timvandermeij

Do you have a PDF file to test this with? Even though the unit tests cover the cases and adding a reference test is not really possible, it would be good for me to verify this in the viewer itself.

src/core/annotation.js

test/unit/annotation_spec.js

src/core/annotation.js

calixteman · 2020-09-19T20:56:21Z

Do you have a PDF file to test this with? Even though the unit tests cover the cases and adding a reference test is not really possible, it would be good for me to verify this in the viewer itself.

https://www.canada.ca/content/dam/ircc/migration/ircc/english/passport/forms/pdf/pptc155.pdf

Save something on page 3 and reopen the newly created file and print.

src/core/annotation.js

Snuffleupagus · 2020-09-20T09:52:32Z

[...] and adding a reference test is not really possible,

Why is that though, since based on the description in #12395 (comment) I'd sort of expect that an eq test of even a single page ought to trigger this if all of the following holds:

Printing is tested, by specifying "print": true, in the manifest.
An empty AnnotationStorage-instance is provided, by specifying "annotationStorage": {}, in the manifest.

timvandermeij · 2020-09-21T21:41:45Z

Why is that though

I thought so because of the "since they were not rendered in annotation_layer" part. If we add a reference test we have to render the page and I thought this bug would not show up then. I might be wrong here and I see that a test is now added, so we should verify if this indeed catches the bug.

timvandermeij · 2020-10-01T20:28:02Z

This PR needs a rebase.

timvandermeij

Finally had some time to review this; sorry for the delay. Looks good with these final comments resolved, the most important one being the reference test that doesn't seem to work.

timvandermeij · 2020-10-07T21:30:40Z

test/test_manifest.json

+       "type": "eq",
+       "print": true,
+       "annotationStorage": {
+         "1605R": true


I verified that this patch itserlf fixes the reported issue, but I can't get this reference test working when checking out this patch and I also don't see how this is supposed to work. The prefilled file seems to only have prefilled content on page 1 and this page is always rendered, so there is an annotation layer and the content is simply in the annotation storage. Checkbox 1605R is also on the first page, so that will also work. I don't see any changes in the output before/after this patch, which confirms my suspicion that this most likely doesn't work as expected.

Did you try the suggestion from #12395 (comment)? You can run the reference tests locally before/after applying this patch to check if the test indeed works as expected.

Since I found that your steps in #12395 (comment) do work, perhaps this test can be made in the same way by providing a PDF with text prefilled on page 3 and then printing it?

timvandermeij · 2020-10-07T21:32:55Z

src/core/annotation.js

    const value = annotationStorage[this.data.id];
+    if (value === undefined) {
+      // No change


undefined actually means "not rendered" because otherwise the value would be false/true depending on the checked state. Given that we also don't have this comment in WidgetAnnotation.save, I'd say we can just remove this line here and in _saveRadioButton below.

timvandermeij · 2020-10-24T17:06:36Z

@calixteman Since form support is getting close to being enabled upstream, it would be great if we can get this PR merged. For the reference test, I recommend to run gulp makeref locally with this branch checked out, with all entries in test/test_manifest.json removed except for the one added in this patch so you can iterate quickly, and then looking at the reference image in the test/ref folder to see what I currently see and if a fix has effect.

calixteman · 2020-10-26T10:00:15Z

@calixteman Since form support is getting close to being enabled upstream, it would be great if we can get this PR merged. For the reference test, I recommend to run gulp makeref locally with this branch checked out, with all entries in test/test_manifest.json removed except for the one added in this patch so you can iterate quickly, and then looking at the reference image in the test/ref folder to see what I currently see and if a fix has effect.

It should be ok now, sorry for the delay.
I ran the test without the patch and nothing is printed on page 2 and it's ok with it.

timvandermeij · 2020-10-26T23:08:25Z

/botio-linux test

pdfjsbot · 2020-10-26T23:08:27Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/4c26b560f7fe076/output.txt

pdfjsbot · 2020-10-26T23:10:51Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/4c26b560f7fe076/output.txt

Total script time: 2.40 mins

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

Image differences available at: http://54.67.70.0:8877/4c26b560f7fe076/reftest-analyzer.html#web=eq.log

timvandermeij · 2020-10-26T23:11:51Z

WARNING: MD5 of file "pdfs/prefilled_f1040.pdf" does not match file. Expected "6c45b2c43e365bb7c88b9cddca0a4865" computed "2335da66fb7c2c3b84971597f27785e2"

Did the PDF file change? If so, the hash also needs to be changed in the test manifest.

…ting

calixteman · 2020-10-27T18:09:07Z

WARNING: MD5 of file "pdfs/prefilled_f1040.pdf" does not match file. Expected "6c45b2c43e365bb7c88b9cddca0a4865" computed "2335da66fb7c2c3b84971597f27785e2"

Did the PDF file change? If so, the hash also needs to be changed in the test manifest.

My bad, I fixed it, sorry.
Anyway the CI is failing but I guess that isn't my fault, else tell me.

timvandermeij · 2020-10-27T21:17:29Z

Oh, that's indeed not the fault of this PR. We'll look into that.

timvandermeij · 2020-10-27T21:37:28Z

Node.js 14 became LTS today, causing the CI builds to fail because the caches still contained packages for the previous LTS version. Purging the cache and retriggering the CI build fixed the issue.

timvandermeij · 2020-10-27T21:37:39Z

/botio-linux test

pdfjsbot · 2020-10-27T21:37:41Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/1e5bacfcc405f09/output.txt

pdfjsbot · 2020-10-27T22:02:37Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/1e5bacfcc405f09/output.txt

Total script time: 24.93 mins

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

Image differences available at: http://54.67.70.0:8877/1e5bacfcc405f09/reftest-analyzer.html#web=eq.log

timvandermeij · 2020-10-27T22:35:57Z

/botio-linux preview

pdfjsbot · 2020-10-27T22:35:59Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/f7a1bbf174f8bef/output.txt

pdfjsbot · 2020-10-27T22:39:56Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/f7a1bbf174f8bef/output.txt

Total script time: 3.94 mins

Published

Viewer: http://54.67.70.0:8877/f7a1bbf174f8bef/web/viewer.html
Viewer (ES5): http://54.67.70.0:8877/f7a1bbf174f8bef/es5/web/viewer.html

timvandermeij · 2020-10-27T23:11:21Z

Thank you!

/botio-linux makeref

pdfjsbot · 2020-10-27T23:11:22Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/3d2676d61a9df20/output.txt

pdfjsbot · 2020-10-27T23:35:22Z

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/3d2676d61a9df20/output.txt

Total script time: 23.98 mins

Lint: Passed
Make references: Passed
Check references: Passed

timvandermeij added annotations form-acroform labels Sep 19, 2020

timvandermeij reviewed Sep 19, 2020

View reviewed changes

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

test/unit/annotation_spec.js Outdated Show resolved Hide resolved

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

Snuffleupagus reviewed Sep 20, 2020

View reviewed changes

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

Snuffleupagus reviewed Sep 20, 2020

View reviewed changes

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

Snuffleupagus reviewed Sep 20, 2020

View reviewed changes

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

calixteman force-pushed the checks branch from 6d2cb18 to 755d04a Compare September 20, 2020 10:50

calixteman force-pushed the checks branch 2 times, most recently from 489a57f to 589b18d Compare October 7, 2020 09:32

timvandermeij reviewed Oct 7, 2020

View reviewed changes

calixteman force-pushed the checks branch from 589b18d to 8fb680c Compare October 26, 2020 09:50

Render not displayed annotations in using normal appearance when prin…

6be2f84

…ting

calixteman force-pushed the checks branch from 8fb680c to 6be2f84 Compare October 27, 2020 18:01

timvandermeij approved these changes Oct 27, 2020

View reviewed changes

timvandermeij merged commit ea4d88a into mozilla:master Oct 27, 2020

timvandermeij mentioned this pull request Oct 30, 2020

Test the windows bot. #12519

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Render not displayed annotations in using normal appearance when printing #12395

Render not displayed annotations in using normal appearance when printing #12395

calixteman commented Sep 19, 2020

timvandermeij left a comment

calixteman commented Sep 19, 2020

Snuffleupagus commented Sep 20, 2020

timvandermeij commented Sep 21, 2020

timvandermeij commented Oct 1, 2020

timvandermeij left a comment

timvandermeij Oct 7, 2020 •

edited

Loading

timvandermeij Oct 7, 2020

timvandermeij commented Oct 24, 2020

calixteman commented Oct 26, 2020

timvandermeij commented Oct 26, 2020

pdfjsbot commented Oct 26, 2020

pdfjsbot commented Oct 26, 2020

timvandermeij commented Oct 26, 2020

calixteman commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

Render not displayed annotations in using normal appearance when printing #12395

Render not displayed annotations in using normal appearance when printing #12395

Conversation

calixteman commented Sep 19, 2020

timvandermeij left a comment

Choose a reason for hiding this comment

calixteman commented Sep 19, 2020

Snuffleupagus commented Sep 20, 2020

timvandermeij commented Sep 21, 2020

timvandermeij commented Oct 1, 2020

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Oct 7, 2020 • edited Loading

Choose a reason for hiding this comment

timvandermeij Oct 7, 2020

Choose a reason for hiding this comment

timvandermeij commented Oct 24, 2020

calixteman commented Oct 26, 2020

timvandermeij commented Oct 26, 2020

pdfjsbot commented Oct 26, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 26, 2020

From: Bot.io (Linux m4)

Failed

timvandermeij commented Oct 26, 2020

calixteman commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Failed

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Oct 27, 2020

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Received

pdfjsbot commented Oct 27, 2020

From: Bot.io (Linux m4)

Success

timvandermeij Oct 7, 2020 •

edited

Loading