Support additional widget actions in PDF Forms
ClosedPublic

Authored by aheinecke on May 2 2018, 2:47 PM.

Details

Summary

This adds support for actions associated with form fields
through corresponding annotation widgets.

Test Plan

Still needs a unit test, only tested manually with
the document attached in the task.

Diff Detail

Repository
R223 Okular
Lint
No Linters Available
Unit
No Unit Test Coverage
aheinecke requested review of this revision.May 2 2018, 2:47 PM
aheinecke created this revision.
aheinecke planned changes to this revision.May 4 2018, 1:51 PM

MouseReleased indeed needs some different handling. It only works in the tests for the Button's as they trigger the "MouseReleased" action as the activation action.

As we need a generic way to trigger activation actions for any Form Element I think it would be best to trigger the "ActivationAction" in the MouseReleased handler for those forms elements that do not have a way to trigger their activation action, yet.

aheinecke updated this revision to Diff 33630.May 4 2018, 2:00 PM

Changed mouseRelease event to fallback to activationAction
Some formatting.

aheinecke updated this revision to Diff 33810.May 8 2018, 7:59 AM
  • Removed a spurious emit when calling signalAction.
  • Updated to API of update poppler patch.
  • Fixed leaking the additional actions.
aacid added a subscriber: aacid.May 18 2018, 12:28 PM
aacid added inline comments.
generators/poppler/CMakeLists.txt
86

You need to move this to a whole new block since it has to be in a check for 0.65 not for 0.64

generators/poppler/formfields.cpp
20

This should be 65

ui/formwidgets.cpp
1073

This still triggers if you press the button inside a form, move the mouse outside the form and release, not sure this is "according to spec", it says "An action to be performed when the mouse button is released inside the annotation’s active area."

1078

Are you sure this should be an else? Why should activation action only be signaled if there's no mouse release action?

Also, before we only did activation action for buttons, but now we do for lots of other forms, is that on purpose?

Restricted Application added a subscriber: okular-devel. · View Herald TranscriptMay 18 2018, 12:28 PM
aheinecke planned changes to this revision.May 28 2018, 10:01 AM
aheinecke added inline comments.
ui/formwidgets.cpp
1073

This still triggers if you press the button inside a form, move the mouse outside the form and release, not sure this is "according to spec", it says "An action to be performed when the mouse button is released inside the annotation’s active area."

Right. I also tested with Acrobat and it only triggers if the mouse is released in the annotation area.

1078

Are you sure this should be an else? Why should activation action only be signaled if there's no mouse release action?

Good question. I checked in the spec and it should be the other way around. I was confused because Adobe Acrobat DC sets the "Mouse Released" action as the "A" entry of the annotation dictonary. This is parsed by poppler as the "Additional Action"

Page 649 (Table 8.44) in the spec Says about the mouse released event:
Note: For backward compatibility the A entry in an annotation dictionary, if present, takes precedence over this entry.

So it should be the other way around. Only execute the mouseReleased action if there is no activation action and execute the activation action otherwise.
I'll change it.

Checkboxes are handled differently because they trigger in the "doActivateAction" so that the action can also be triggered by scripts changing the checked state.

Also, before we only did activation action for buttons, but now we do for lots of other forms, is that on purpose?

Yes this is on purpose. The activation action is not bound to buttons. I did not find it clearly stated in the spec but If I add a Mouse Release action on a textfield in Acrobat DC it is added as a Mouse Release action.

And the Example from bug306818 uses a mouse action on a read only text field to hide the warning in there.

aheinecke updated this revision to Diff 35014.May 28 2018, 10:18 AM
  • Fixed precedence of activation action over mouse release action.
  • Only activate mouse released if release is inside the widget.
  • Fixed check for Poppler 0.65
aheinecke added inline comments.May 28 2018, 10:26 AM
generators/poppler/CMakeLists.txt
97

Out of curiosity: Why is this and the other "check compiles" tests for released versions of poppler not:

if (Poppler_VERSION VERSION_GREATER "0.64.99")
  set (HAVE_POPPLER_0_65 1)
endif()
aheinecke updated this revision to Diff 35015.May 28 2018, 10:41 AM

Don't eat the MouseRelease event if it is outside the widget

aacid added inline comments.May 28 2018, 2:13 PM
generators/poppler/CMakeLists.txt
97

In this case you could do it (i assume the syntax is correct), but most of the times the feature is developed both here and in poppler so at that point there's no actual poppler version out yet, so you can't depend on a version number that doesn't exist.

ui/formwidgets.cpp
1078

Can you please add that explanation either as a comment somewhere in the code or as part of the git commit? It'll make it easier to find next time someone looks at the code and wonders why it has an "else " like i did.

aheinecke added inline comments.May 28 2018, 2:35 PM
ui/formwidgets.cpp
1078

In the comment in line 1052ff I tried to do just that. A problem with the macros is that you cant properly do inline comments.

aacid accepted this revision.May 28 2018, 3:17 PM

It would defenitely be nicer using a template for this instead of the big define, but if you already tried it i don't really object much, i'm fine with this and will commit it (or you can) once you fix the remaining comments for the associated test

This revision is now accepted and ready to land.May 28 2018, 3:17 PM
This revision was automatically updated to reflect the committed changes.