[api-minor] Include export value for checkboxes #9874
Conversation
bion
force-pushed
the
master
branch
2 times, most recently
from
ae59011
to
24749d0
Compare
|
@timvandermeij can this be merged? |
src/core/annotation.js
Outdated
| if (!isName(this.data.fieldValue)) { | ||
| return; | ||
| } | ||
|
|
||
| var customAppearance = params.dict.get('AP'); |
There was a problem hiding this comment.
Change var to const.
src/core/annotation.js
Outdated
| var customAppearance = params.dict.get('AP'); | ||
|
|
||
| if (customAppearance) { | ||
| let exportValues = customAppearance.get('D').getKeys(); |
There was a problem hiding this comment.
Change let to const.
src/core/annotation.js
Outdated
| if (customAppearance) { | ||
| let exportValues = customAppearance.get('D').getKeys(); | ||
|
|
||
| this.data.exportValue = exportValues[0] === 'Off' ? |
There was a problem hiding this comment.
I would like to have the unit tests extended so that we can be sure that this functionality does not break in the future. They can be found in test/unit/annotation_spec.js.
There was a problem hiding this comment.
Furthermore, this patch doesn't appear to do anywhere near enough validation of the structure of this data; doing similar checks as in the _processRadioButton method just below would seem like a requirement to prevent unnecessary future bugs.
bion
force-pushed
the
master
branch
6 times, most recently
from
a69269f
to
08c8baa
Compare
|
Hi @timvandermeij and @Snuffleupagus, I added an additional test case and validated the structure of the data as per @Snuffleupagus's recommendation to follow the pattern in |
|
Code-wise this looks good, but I'm still a bit confused about the functionality. How should this work when the PDF is rendered? I tried making multiple checkboxes with the same name, but that doesn't make them act as radio buttons in HTML (not without some JavaScript at least). I understand that this patch makes the export value available in the API, but I think we should also have a consistent UI for this when we render them. They are rendered at https://github.com/mozilla/pdf.js/blob/master/src/display/annotation_layer.js#L491-L517; note how they have no name. In short, this patch makes it so that the export value of a checkbox is the state name other than the |
|
@timvandermeij This will not result in a change for the default annotation rendering. This change is useful in cases when the API consumer is interested in which checkbox has been interacted with by the user. Exposing the export value answers the question of "what is the semantic meaning of this checkbox". Our use-case at Boundless is an example of how this is useful. In our UI the user's interactions with the annotation layer are used to generate PDFs with the AcroForm fields filled out. If you look at the USCIS I-130 you'll see that question 1 on page 1 is a set of four checkboxes, all with the same field name, but each with a different export value. Exposing the export value to the annotation layer allows us to provide a UI that can interpret the semantics of the user's interaction with those checkboxes. |
|
Thank you for the clarification. In the USCIS I-130 PDF file, it mentions "Select only one box" for question 1 on page 1. To me this implies that the user could in fact select multiple, but is asked to only select one. Is it therefore OK to not change the default rendering and just render multiple checkboxes that may be checked, and leave it up to the API consumer to be able to work with multiple selections if the user does so? I.e., we don't have to enforce the "act as radio buttons" part in the UI; is that what other viewers do? |
|
Moreover, could you amend the commit message (using |
timvandermeij
changed the title
|
@timvandermeij Updated the commit message per your instructions. Re: the checked button behavior, the USCIS I-130 PDF is often printed on paper which is likely the reasoning behind their explicit instructions.
Adobe Acrobat and Adobe Reader enforce the radio-like behavior but macOS Preview and Google Chrome do not. Given the discrepancy there I think leaving it up to the API consumer makes good sense. |
|
/botio test |
From: Bot.io (Linux m4)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.67.70.0:8877/c6cc2b091ea4664/output.txt |
From: Bot.io (Windows)ReceivedCommand cmd_test from @timvandermeij received. Current queue size: 0 Live output at: http://54.215.176.217:8877/7a1e788342eb72b/output.txt |
From: Bot.io (Linux m4)SuccessFull output at http://54.67.70.0:8877/c6cc2b091ea4664/output.txt Total script time: 19.53 mins
|
From: Bot.io (Windows)SuccessFull output at http://54.215.176.217:8877/7a1e788342eb72b/output.txt Total script time: 26.85 mins
|
|
Looks good. Thank you for your contribution! |
|
@timvandermeij thanks for supporting the change and getting it in! |
Fixes #8095
U.S. Government forms often use checkboxes as radios by specifying different export values of checkboxes with the same name. See the USCIS I-130 as an example.
Adding the
exportValueproperty to checkbox annotation widgets enables correct rendering of checkbox state for forms that do this.