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.

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

Add support for ResetForm Action #7412

Closed
wants to merge 1 commit into from

Conversation

jsawruk
Copy link

@jsawruk jsawruk commented Jun 14, 2016

See PDF 32000-1:2008, 12.7.5.3 Reset-Form Action (p. 455).

This will be used to support clearing a form's values when viewing a form using pdf.js. It will enhance the functionality of the pdf.js acroforms demo (examples/acroforms).

if (data.fieldType === 'Btn' && dict.get('A') !== undefined) {
var actionRef = dict.map.A.num;
var xrefObject = dict.xref.fetchIfRef(dict.map.A);
data.formAction = {};
Copy link
Member

Choose a reason for hiding this comment

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

This formAction property seems not used in src/ or web/. How does this addition introduce support for the ResetForm action?

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, this is probably leftover code from something else I was working on. It will be removed.

Copy link
Author

Choose a reason for hiding this comment

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

No, this is not an unused variable. See line 610. I am then using this code to determine what type of annotation this is. Example code:

if (item.formAction.S === "ResetForm") {

Copy link
Member

Choose a reason for hiding this comment

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

I can see that you're setting another property on data, but what I'm saying is that after setting the property, the property is not used elsewhere.

Taken in isolation, it seems that this extra property doesn't do anything. It is not documented anywhere as far as I can tell, and neither used by the pdf.js code (web/ and src/).

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I understand that. You are correct that it is not used in web/ or src/. The only place it would be used is in the examples/acroforms. Would you like me to update the code in examples/acroforms? It is my understanding that this pull request is on hold until #6172 is landed. Please let me know how you would like to proceed.

Copy link
Member

Choose a reason for hiding this comment

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

I concur, let's wait until #6172 lands.

@Rob--W Rob--W added the core label Jun 14, 2016
@timvandermeij
Copy link
Contributor

timvandermeij commented Jun 15, 2016

Since there are many types of Widget annotations, we should use an approach like in #6172. That is a much cleaner way to expose the data through the API. Especially since there are many Widget annotation types, I think it's key to set up a good, reusable structure for them such that it will be easy to add support for new types if necessary. I want to avoid going back to older versions of annotation.js where we had all kinds of if statements handling stuff without a clear structure.

We should probably review/land #6172 first so it can be a basis for this PR. Unfortunately I do not have time at the moment to do so...

@jsawruk
Copy link
Author

jsawruk commented Jun 15, 2016

@timvandermeij: I wasn't aware of #6172. I was trying to solve a specific issue where I work, hence my smaller and very specific pull requests. I agree that a reusable structure would be preferred, but I didn't have the bandwidth to create something as comprehensive as #6172. Clearly there is a need for these features, and I would like to see #6172 be a higher priority. If #6172 is landed and covers the functionality that I need in my project, then there is no need for my pull requests. Thanks for informing me about #6172.

@timvandermeij
Copy link
Contributor

timvandermeij commented Sep 12, 2016

Closing as incomplete. We are tracking this in #7613. Form actions have not been implemented yet and therefore this PR currently has no effect. The structure required for this is not yet in place, but will require code changes. This PR may be used as inspiration for future PRs attempting to implement this behaviour, but in its current state we cannot merge it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants