Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix AcroForm printing/saving edge cases #12263

Merged
merged 5 commits into from
Aug 23, 2020
Merged

Fix AcroForm printing/saving edge cases #12263

merged 5 commits into from
Aug 23, 2020

Conversation

timvandermeij
Copy link
Contributor

The commit message contain more information about the individual changes.

Fixes #12233.

@timvandermeij
Use the export value instead of the display value for choice widget o…
5fed711 
…ption selection

The export value is used when the document is saved, so it should also
be used when the document is opened to determine which choice widget
option is selected. The display value is, as the name implies, only to
be used for viewer display purposes and not for other logic.

This makes sure that in the document from mozilla#12233 the "Favourite colour"
choice widget is correctly initialized with "Red" instead of "Black"
because the field value is equal to the export value (always the case),
but not the display value (not always the case). Moreover, saving now
also correctly uses the export value and not the display value.
@timvandermeij
Unconditionally set the field value for choice widgets in the annotat…
36e1498 
…ion storage

This commit makes the following improvements:

- The code is similar to the other interactive form widgets now, with a
  clear note for the only difference.
- Calling `getOrCreateValue` unconditionally ensures that choice widgets
  always have a value in the annotation storage. Previously we only
  inserted a value in the annotation storage when an option matched or
  when a selection was changed. However, this causes breakage when
  saving/printing because comboboxes, which we don't fully support yet
  but are rendered, might not have a value in storage at all. Their
  field value might not match any option since it allows the user to
  enter a custom value.
- Calling `getOrCreateValue` unconditionally ensures that forms with
  choice widgets no longer always trigger a warning when the user
  navigates away from the page. This fixes
  mozilla#12241 (comment)
@timvandermeij
Copy link
Contributor Author

/botio-linux preview

@pdfjsbot
Copy link

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/e82e8c9a919525a/output.txt

@timvandermeij timvandermeij changed the title Fix AcroForm printing edge cases Fix AcroForm printing/saving edge cases Aug 22, 2020
@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

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

Total script time: 3.31 mins

Published

@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/8cb713621af7408/output.txt

@pdfjsbot
Copy link

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/eaa6c3d34e10719/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

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

Total script time: 27.28 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/8cb713621af7408/output.txt

Total script time: 29.55 mins

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

Image differences available at: http://54.215.176.217:8877/8cb713621af7408/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator

Also, looking at the test-results for annotation-button-widget-print-page1 test-case:

  • The "Check box, unchecked"-line is no longer checked, which seems correct (as far as I understand).
  • The "Check box, checked"-line on the other hand is not checked; shouldn't it be, or is that related to the remaining issues mentioned in one of the code comments?

@timvandermeij
Copy link
Contributor Author

[...] shouldn't it be

You're right. It turns out that the N entry in this case only has one of the two states (/Off is missing), and D has the both. I'm checking the spec on why that is since I thought they always had to be defined, but it could be that /Off is a default state and may be left out.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Aug 22, 2020 •
edited
Loading

The appearance for the off state is optional but, if present, shall be stored in the appearance dictionary under the name Off. (https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#page=448&zoom=auto,-215,717)

Well, there's the answer. I'll update the patch; thanks for noticing!

@timvandermeij
Copy link
Contributor Author

I have addressed all comments; see https://github.com/mozilla/pdf.js/compare/302007fdd1d2da34e93a628fb5948fbc7add9169..917fbbe5e207b94d78ad8ff39cd8b464db3c7a8c for the interdiff. I also found that I accidentally added the test file to .gitignore even though it's a linked file, so that is fixed as well. I have verified that the reference test failure is now fixed and the document from the issue still works the same.

/botio test

@pdfjsbot
Copy link

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/67a44ed4e98c4b1/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/bacb6a3e6844c11/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/67a44ed4e98c4b1/output.txt

Total script time: 27.12 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/bacb6a3e6844c11/output.txt

Total script time: 29.33 mins

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

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

Snuffleupagus
Snuffleupagus approved these changes Aug 23, 2020
View reviewed changes
Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, modulo a couple of comments; thank you!

@timvandermeij
Decode widget form values consistently
1b82ad8 
The helper method `_decodeFormValue` is used to ensure that it happens
in one place. Note that form values are field values, display values
and export values.
@timvandermeij
Obtain the export values for choice widgets from the normal appearance
a8efc02 
The down appearance (`D`) is optional and not available in the document
from mozilla#12233, so the checkboxes are never saved/printed as checked
because the checked appearance is based on the export value that is
missing because the `D` entry is not available.

Instead, we should use the normal appearance (`N`) since that one is
required and therefore always available.

Finally, the /Off appearance is optional according to section 12.7.4.2.3
of the specification, so that needs to be taken into account to match
the specification and to fix reference test failures for the
`annotation-button-widget-print` test. That is a file that doesn't
specify an /Off appearance in the normal appearance dictionary.
@timvandermeij
Include forms/print reference tests for the document from mozilla#12233
73bc8e1 
In addition to the unit tests these reference tests make sure that this
document, that triggered some edge cases in our code, can be rendered
and printed successfully now.
@timvandermeij
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/1f58f02a189f55f/output.txt

@pdfjsbot
Copy link

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/6f72d8d6f3e20d9/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/6f72d8d6f3e20d9/output.txt

Total script time: 26.95 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/1f58f02a189f55f/output.txt

Total script time: 29.08 mins

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

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

@timvandermeij timvandermeij merged commit 7df8aa3 into mozilla:master Aug 23, 2020
@timvandermeij
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

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/9ee69b24bd625f0/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/bdc53727e0369f3/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/9ee69b24bd625f0/output.txt

Total script time: 25.48 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/bdc53727e0369f3/output.txt

Total script time: 28.94 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@timvandermeij timvandermeij deleted the acroform-fixes branch August 23, 2020 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snuffleupagus Snuffleupagus Snuffleupagus approved these changes

Assignees
No one assigned
Labels
annotations form-acroform printing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Checkmarks are not shown in Print Preview and are not printed in pdf
3 participants
@timvandermeij @pdfjsbot @Snuffleupagus