mozilla / pdf.js

PDF Reader in JavaScript
https://mozilla.github.io/pdf.js/
Apache License 2.0
48.34k stars 9.97k forks source link

Radio options not rendered when save document #15096

Closed jegathesan closed 2 weeks ago

jegathesan commented 2 years ago

Sample File: SmartFormAllFields.pdf

Configuration:

Steps to reproduce the problem:

  1. Use above sample pdf.
  2. Modify radio options

What is the expected behavior?

  1. Only selected radio button should checked when saving document using saveDocument()

What went wrong?

  1. When save document radio button not selected properly Screenshot 2022-06-24 at 10 14 18 PM
calixteman commented 2 years ago

The two radios (62R and 66R) don't have a P entry, hence their pageIndex is -1: https://github.com/mozilla/pdf.js/blob/cd35b9bfac85c00238cf0fa0f657a780f2a5ead6/src/core/annotation.js#L225 and when a radio is clicked we must store its value and the one for its siblings in the annotationStorage but when try to collect the siblings the object are just skipped because of the pageIndex: https://github.com/mozilla/pdf.js/blob/cd35b9bfac85c00238cf0fa0f657a780f2a5ead6/src/display/annotation_layer.js#L521

calixteman commented 2 years ago

According to the specs the P entry is optional, so we should find an other way to get the pageIndex.

@Snuffleupagus, any ideas ?

Snuffleupagus commented 2 years ago

and when a radio is clicked we must store its value and the one for its siblings in the annotationStorage but when try to collect the siblings the object are just skipped because of the pageIndex:

https://github.com/mozilla/pdf.js/blob/cd35b9bfac85c00238cf0fa0f657a780f2a5ead6/src/display/annotation_layer.js#L521

Do we actually need to check the page-index there, or could we just remove that check?

More generally: where do we actually need the page-index, in the scripting implementation?

calixteman commented 2 years ago

There is a page property in the Field object: https://opensource.adobe.com/dc-acrobat-sdk-docs/acrobatsdk/pdfs/acrobatsdk_jsapiref.pdf#page=433&zoom=auto,-106,711

You added this page check: https://github.com/mozilla/pdf.js/pull/14023

I think it should be fine to remove it, we just "need" (I'm not sure to be able to imagine a form where using this property could be really useful) to find a way to get the number.

Snuffleupagus commented 2 years ago

You added this page check:

14023

Whoops; unfortunately I didn't have a good reason for doing that, but rather figured that it'd make sense (without really knowing anything about the scripting-side of things).

MallRoy commented 2 years ago

We are also facing the same issue. any update on this @Snuffleupagus @calixteman?

calixteman commented 2 years ago

This pdf is a corrupt: in Root::Fields, the object 10R has 2 children 62Rand 66R but those ones don't have aParent entry which is this case required (table 220): https://christianhaider.de/dokuwiki/lib/exe/fetch.php?media=pdf:pdf32000_2008.pdf#page=440&zoom=auto,-147,370